 Hello, everyone. I'm happy to introduce Adam Dengueur. He's a Python developer coming from Bristol. And he will present a talk about another pair of eyes reviewing code well. Thank you. Thank you, everyone. Hello. I'm Adam. And I'm going to talk about code review. But why am I talking about code review? Well, the inspiration for this talk is really a bunch of people I used to work with who are mostly contributors to the Twisted project. And I was really lucky. I got to experience the really rigorous approach to review that's also present in Twisted. And I feel that I've become a much better programmer thanks to that. And I've also had some really bad code review experiences, the kind of experiences that make you not want to go into work the next day. And I never, ever want to make someone feel like that. But when it's your role to judge someone else's work, it's really easy to upset them if you're not careful. And I guess that some of you have experienced with code review maybe even most because it's kind of coming a fundamental part of the software engineering process. But maybe because of that, it's often seen as boring grunt work and not a skill that you can hone and get better at. But at my job, I spend almost a third as much time reviewing code as I do writing it. But when I go to programming forums or Twitter or read blog posts about how to become a better programmer, I see very, very little about how to become a better reviewer. And when a programmer starts out, they're often paired with a mentor to teach them how to code or given kind of like easy fix bugs that they can get trained on. But we're really expected to jump in at the deep end and know how to review code without ever having been taught. So today, I'm going to talk about these points, what code review is, how I review code, some common pitfalls and traps that people fall into when they're reviewing, tools which can help make your life easier and how code review can make you a better programmer. So what is code review? It's a practice where someone looks over someone else's code and they look for things which can be improved. And we usually do this before merging code because we want to catch bugs as early as possible. Yes, we've got automated tests, but code review can sometimes be the final gatekeeper before code hits the real world. And we also use it to catch non-bug defects which can harm the project, such as performance issues or maintainability problems or something like that. But code review isn't just about making the one change set that we're reviewing better, it's about making all of the software that we ship better. And it's important to think about how we can use code review to improve ourselves and the contributors that we work with. And I saw a talk last year at the DOX conference by Paul Rowland, who I think is also at EuroPython, and he was talking about how we should stop trying to be code ninjas and rock stars and all those other kind of things that you see on job interview, on job posts, and we should start trying to be code nurses and code tree surgeons. And I kind of see the reviewer's role as just like that. It's not as thrilling as the ninja feature author shipping something at midnight, but it's really important to the health of the project. And all of this happens within the context of the project's goals, which usually includes shipping code fast or at least fast enough. So let's back up and go over quickly how we get to the review stage. And in the last few companies I've worked at, it goes something like this. Eleanor, our programmer, is new to the project. She's brand new and she's looking to make her first contribution. So she picks a task from the task tracker. I'm using GitHub here, but it could be JIRA, Trello, whatever you use. And this one is written by me. The name of the user's latest employer needs to be shown in our apps new prototype feature, user profiles. So she creates a branch so that she can make changes to the code without worrying about breaking it. And using branches for development is a great way to create like a low risk environment for people to experiment in. And it lets a reviewer easily look at the proposed change, just the difference between what's being written and what's currently there. And so Eleanor starts searching around the code and she finds that there's already a method to get date sorted employment history, great. And there's already a function that you can see here that returns a dictionary for a user's profile that's later handled by like a view layer and shown in the app. So she adds some new code that she thinks might work. You can see those new two lines. And it takes the date sorted employment history and it displays the first item of that list in a new field called latest employer. And then she runs the application locally and she checks her own profile and it shows her latest employer, Bill Bow Inc. So we know that this works. Eleanor has seen that it works. Do we need to spend time reviewing it? Well, in my opinion, all code that's written as part of a team should be reviewed. I've heard objections to this, particularly when the change set is trivial or if it's absolutely urgent. And I'm going to propose that very, very little of what we do is actually trivial. Programming is very hard, at least for me, and authors have an inherent bias towards thinking that their code is good. Even if we waste a few minutes reviewing a change that really is trivial, we save hours later debugging the bug that was shipped when someone thought their code was really simple, but it actually had a typo. And even if we're absolutely certain that the code can't break, a review means that at least two people know where this new code is. And if the code is really urgent to ship and there really isn't someone around to review it, maybe it is worth merging. Maybe you've shipped a data destroying bug and you really need to revert it right now, but at the very least I'd recommend that you do a post-merge review. Whatever that is, I won't go into it. So, back to Eleanor. She submits her code for review and she's nervous because it's the first contribution to the project that she's made. But of course she doesn't mention that and she puts the code up for review. So, now we've got to decide who does the review. Who gets permission to merge code is a whole problem that I won't go into, but it's got to be someone with that permission. But at least where I've worked, anyone can do it. Anyone can review code and merge it and it's just like a trust-based system. And as I mentioned, as programmers, we're biased towards thinking that our code is working, but we're also biased towards thinking that our code is readable. Our comments are readable, our variable names are understandable. So, the ideal reviewer is someone who has had no part in writing the code. And if we choose someone who's recently worked on the changed files, then they're likely to have really great context which will help them find a high number of bugs. And according to a Microsoft study that I read, the most useful review comments, so I guess the most bugs found in most cases come from people who've actually previously reviewed the modified files. But when we choose people who are recent authors or recent reviewers, that conflicts with our goal to share knowledge with the team. And that might be okay, but it's at least something to consider. But a good workaround for that problem is when you allow developers who are unfamiliar with the code to do a review, but you don't designate them as the gatekeeper. You have someone else have to say, this looks good to me, let's merge it. In fact, when I join a team, I love to jump straight into reviewing as much code as possible. And when there's no pressure of knowing that I have to take responsibility, I'm not the one saying it has to look good. Someone else will do that. Then it's a great way to learn how things are done. It teaches you, let's say, about a project's quality bar and standards and how the process works from writing code all the way through to shipping it. So say I want to review Eleanor's changes. This is how I'll go about it. I start by reading the spec. Without reading the spec, there's no way that I can know if the code meets all the requirements. Hopefully the spec's in an issue tracker and it has descriptions and examples of how the new functionality might be used or how to reproduce the bug if there's a bug that we're fixing. And then I check the CI, whatever that is, whatever tool you use. And I hope that it's passing, but sometimes we know that CI isn't always passing, so at least I check that there's no new issues. And CI really should be a signal of whether something that was already there is broken, so it's good to check that first. And then next, I look for evidence that the spec's been implemented. And ideally, that means looking at the test changes. If there's a bug to be fixed, I want to see that there's a passing test which would have failed if the bug were present. And if there's a new feature, then I want to see tests that cover the whole spec. And if it seems like the spec is ambiguous and I've interpreted it differently to the author, then this is a good time to start a discussion about what we want. That could be with the author or whoever's responsible for shipping the product, maybe the product owner or project manager, whoever it is in your organization. And then I look at the implementation. I think about whether it has functionality that isn't tested. And that's important because even if the code works now, we want it to be safe from regressions. And I kind of think about which missing test cases could convince me of that. And then I think about whether there's any risk that the changes could break existing functionality. Yes, we've got CI, but it's not always perfect and it's often a good thing to think about. And then I think about, is the new code going to be easy to maintain? And that can range from things like, you know, other variable names understandable, but also I have to think about what the structure of the code is. Is it really branchy? Does it have lots of side effects? Is it idempotent? Is new code in the place that I'd expect to find it? And these are all the things that you think about when you're writing the code. So you get a lot of the advantages of pair programming, but it's asynchronous. And then I check that everything user facing is documented. That might not be relevant in your project, but it is in mine. And also at the same time, while I'm reviewing the code, I'm looking out for new techniques and patterns and tools that I don't know yet. And it's always really nice to say thanks to the author when you learn something from reading their code. And then I'll write a comment about anything that might be improved. And I'll ask questions and start a discussion about anything that I'm unsure about. And at the end, I'll give a conclusion. And usually it's one of these options. I say, great, it looks good to me, I'll merge it. Or please make the requested changes and then you can merge it or make the requested changes and then resubmit for another review. And then sometimes in the rare case, I've picked up a review that I'm not qualified for and I asked to get someone else looks at it. And I'd say that the first step to getting better at reviewing all those previous steps I've mentioned is to take it seriously, like we often do the rest of our craft. If you learn about software engineering practices from blog posts, then search for blog posts. If you learn from formal studies, then look for formal studies about code review. And importantly, ask for feedback on your reviews, just like you ask for feedback on your code. So I look at Eleanor's new code and I immediately spot a problem. The Twitter handle feed just above is title case, but the new field is sentence case. And so I know that the pull request is not up to scratch and I comment and reassign the issue to Eleanor. So what are her next steps? Well, she sees my comment and she kicks herself for making such a trivial mistake. Why can't I do anything right, she thinks. Maybe I'm not good enough to be here. And then she searches the code base for latest employer, the name of the new field. And she sees that it's written just like she wrote it. Everywhere else it comes up in the code. Beginners don't have the confidence to protect themselves from feeling hurt by code review. And some experienced developers don't either. So what we should do as a community is be wary of imposter syndrome and use code review as a forum to make this industry a nicer place. But you don't want to let bad things be merged just to avoid hurting someone's feelings. One tip is to always say one nice thing about the code. And when someone's made an effort, there's always something nice you can say. And it's good to always address the patch and not the person. At a previous company, I was lucky enough to be thrown in at the deep end of a project. And that project had quite a steep learning curve. But it took me some time to get into the mindset that my code was being reviewed and not me personally. But what else could be better with this review? Well, Elena doesn't know what to do next if you remember that she was searching around the code base. And one of our goals, the project's goals, is to ship code quickly. So by not making it clear exactly what she's got to do next, I've slowed down the process and I've added unnecessary work for her. And also, you might spot that there's a bug a few characters later. Basically, if that list is empty, get employers, then I'm gonna get an index error when I try to access the first item. But I stopped my review when I saw the first problem. And that means there's got to be a whole extra round of review which delays our shipping the code. And of course, sometimes there's a trade-off when you're thinking about when to stop. If a patch is really large and based on a completely wrong assumption, then scrutinizing every line probably isn't worthwhile. So, after a back and forth and a comment about the index error and so on, Elena resubmits this code. And I'd say that's a code of review success. We stopped a bug getting into production. It was great. And so I start my next round of review and I comment that a try accept clause has performance overhead that we don't want. And the message line is greater than 80 characters and that violates pep eight. And finally, I mentioned that using a getter for the list of employers to remember that function she found and used isn't very nice and she should use a property instead. And on the surface, these might seem like reasonable comments but it really is not a great review. Because as a reviewer, I need to consider the context and I need to consider what's important right now. Sometimes it's security, sometimes it's maintainability, sometimes it's speed, who knows. And this right here is prototype code if we remember the spec. So there's no need for me to be overly concerned with tiny, tiny performance issues. So I've kind of wasted some time with that suggestion. And that's for pep eight. Well, it's a guide. It's not a rule set. And if you want to follow a guide, fine. But if you want rules, then I totally recommend using a tool or set of tools to enforce them. And that's partly because humans are very bad linters. We miss things that tools catch. And it's partly because if we use tools, then we can free ourselves up to do more important things. And flake eight is one of my favorite tools. It combines a lot of tools and it enforces the flake eight guidelines, some of them as rules. And then there's coverage.py. It checks your line coverage and requires.io which checks for like vulnerabilities, dependencies and landscape.io which looks for common code smells and mistakes that people often make. And there are even tests for docs. If you're using restricted text, there's doc eight, which lends your documentation. And there's Sphinx's link checker and spell checker. And there are probably similar checkers for Markdown and ASCII doc and whatever tool chain you use. But sometimes these tools will suggest changes that are actually worse. My personal preference is just to always just follow the tool suggestions. And overall, your code is probably going to be nicer and you save the cognitive burden of linting by eye. But if you really want, most of these tools allow you to ignore a particular issue. Like here, I've told Pylon, don't worry that the function name is too long. And if a suggestion keeps coming up in your project and your team don't like it, you can just disable some of the checks in most of the tools or change them. And if you use GitHub, then you can integrate these tools with pull request so you can actually make it so that code can't be merged if the tools find problems. And finally, reviewable is a really powerful tool which has great features that GitHub doesn't have. Like, say you're doing a review, you can say I just want to see the changes since I last did a review. So if we go back to my review, we see a suggestion of an unrelated change, changing an existing getter to a property. Now, if you want an unrelated change, it's probably best to just make an issue in your issue tracker and deal with it later. A review might be a great place to spot required unrelated changes, but not to request them. Because asking for unrelated changes slows down shipping the code and also makes the next review round less pleasant. In fact, as a reviewer, I should request easily reviewable changes. But what are reviewable changes? Well, statistically, more defects are found when change sets are shorter. And as a reviewer, you can ask that a patch is split up if it's difficult to review. And you might be able to help decide where the splits should be. So say someone refactored some code before adding to it, you might ask that the refactoring is in its own separate pull request. And when you spend a lot of time reviewing code, you start to get a good idea of what's going to be easy to review, and you'll start to write code like that. And writing easy-to-review code means reviewers can be more effective and find more bugs so your software's better. And when we focus on making reviewers life easier, we write code that can be extended without changing what's there too much, otherwise known as maintainable code. And a trivial example of this is the Silicon Valley Comet. If I add a Comet at the end of that line, then when I add a new item, I only have to see, when I add a Comet at the end of Basilica, when I add a new item, I get to see just one new item's changed. Great. And the same for docs, but I'll skip it because we're running low on time. And I'm gonna say that the best reviews aren't just laser-focused on providing the bare minimum needed to ship, but they also don't add unnecessary blockers to merging. And when you explicitly state that a suggested improvement is optional, and you can let the author choose whether, let's say, to learn a suggested tool or to just skip it and ship now, then that can be really useful. And those priorities can be adjusted depending on the context. And let's say if you've got a brand new developer to the project, you're onboarding someone, well, you can be super piggy and get them in sync with the rest of the team, or you can be easy on them because you want them to feel happy and get started quickly. And so every review comes with a mini, how would I have done this? When we read someone else's approach to the answer to that question, and we notice that their approach is better, or at least better in some way, then we learn techniques and patterns from them. And I find that teams which value review get really much better reviews back, and they get reviews done more quickly. And my favorite example of this is Twisted's High Scoreboard. So you get a score every month and you get some points for submitting, but you actually get the most points for doing a review. Thank you very much for having me. I'll also, I haven't got to put them up yet, but I'll put my slides up there, and I'll probably tweet about it or something, hopefully when I link later. So are there any questions from the room? We have a few minutes for questions. I have just a quick one. What's that score thing that you showed in the end? That's Twisted. So Twisted is an open source project. It's like a networking framework, and they want to encourage people to contribute. So just for a bit of fun, they've got a high scoreboard and says who's been the best contributor this month or the most prolific contributor this month? So you get like 100 points or something, if you, let me go back to it. Oh, where were we? Yeah, if you're 200 points, if you submit some code, 500 points if you close a ticket, but you actually get 1000 points if you do a review because they have a backlog of code that's been submitted and not reviewed. And so there's no point to having new code if the code that's already been submitted isn't merged. Hi, great talk. Thank you. So when you review a code, do you also suggest to check out it and test it? Like if it's more complex? Or is that not part of your job as a reviewer? How do you stand on this? So I don't always do it. Sometimes the tests hopefully tell me enough. If there are no tests, sometimes there's a good reason that code isn't tested. Let's say it's prototype code we're gonna throw it away, or maybe it interacts with an external service and it would just be really costly to build a fake for that service. What I like to have is a reproducible test, manual test case, so let's say if it's elastic search, connect to elastic search, check this in the web interface after you run my code or whatever. And I like to get my editor out, I like to get my web browser out, run it and check that that works. And also if I've got a suggestion and I'm not like 100% sure about it, I like to modify the code, see if my suggestion might work, at least in a prototype stage, or see if I can break the tests by modifying the implementation or make the test still pass by modifying the implementation in a way that the functionality would break and then I've found a flaw in the tests. So I do get my editor out. Thanks for the great talk. I have a question regarding the size of reviews, how to handle it, because if you have a feature that needs to be implemented, but the change that would be too large, how do you handle a case like that? Do you make earlier pull requests with only a partial implementation of the feature? Or what would you suggest? So I like to always keep master or trunk or whatever you call your main branch working. And so one way that people do it is you make a branch for the whole feature and then that's just empty, it's like master. And then you make branches off that which have all the partial features and you put those branches into that branch, the first branch I mentioned, I should have given them names and then eventually when you're happy with it all, you've got this huge diff and you've merged that into master. Hi, Mika. What about time spent? What do you suggest? Like you can go really in deep, like reviewing every line, texting every line. Cool. So it depends on a few things. One, I never want to be tired in a review. As soon as I'm tired, maybe like 45 minutes in, my review is useless, I'm gonna miss bugs and stuff. But as for scrutiny, how it really depends on the context again. If I'm reviewing something right at the core of my project, it needs to be great, I'll spend as much time as needed. Or if it's, let's say, an API, that's going to be really tough to change later because users are going to depend on it, then I'll also be really, really deep. If it's something in the middle or like I mentioned before, prototype code or something like that, then I can say more like trust or, okay, I've opened this up in a web browser and it all seems to work so fine or at least something a bit closer to that. And like I said at the end, if it's a beginner programmer on your team, then maybe you want to be really picky just to help them along or something like that. It depends on the person as well. Last question. Oh, sorry. So imagine you have like several issues and all those issues can be resolved in maybe one or two lines of code. Do you favor like several commits or just one commit? So different projects do it in different ways. Some people like to review the commits. It's quite nice when you've got a set of commits to be able to just revert one or to see a history. Why was that? Why did that come up? That's the advantage of review of commits but usually in my reviews, at least where I work now, I don't deal with commits. So that's a different practice. Lots of people do, but I don't. So I'm sorry, I don't have a great answer for that. So we are running out of time. Thank you very much for your insights. Thank you. Thank you.