Another pair of eyes: Reviewing code well
Dangoor, Adam
Adam Dangoor - Another pair of eyes: Reviewing code well Many of us have been taught to code, but we know that software engineering is so much more than that. Programmers can spend 5-6 hours per week on code review, but doing that is almost ignored as a skill, and instead it is often treated as a rote chore. How many of us have seen poor reviews - those which upset people, don't catch bugs or block important features being merged? This talk explores the social and technical impacts of various code review practices as well as helpful tooling. The goal is to provide a structure to help improve how teams review code, and to introduce the costs and benefits of code review to anyone unfamiliar with the practice. There are always trade-offs to be made - e.g. think how costly a security flaw in this code could be to your organisation - perhaps intense scrutiny is not necessary for prototypes soon to be thrown away. It is useful to consider the trade-offs in order to optimise for a particular problem domain. Perhaps right now it is more important to look for issues with maintainability, functionality or performance. I talk about how some fantastic code reviews from mentors, colleagues and strangers have helped me become a better programmer and team member, as well as occasions where code review has been detrimental by slowing things down and causing arguments. This is aimed at everyone from beginner to advanced programmers.
 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 or all those other kind of things that you see on job posts. And we should start trying to be code nurses and code tree surgeons. And I kind of see the reviewers 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 Gira, 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 app's 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 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 been 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 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 Bowink. 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 Alana. 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, they're conflicts with our goal to share knowledge with the team. And that might be OK, 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 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 ask that someone else looks at it. And I'd say that the first step to getting better at review and all those previous steps I 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, Eleanor 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 going to 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, Eleanor resubmits this code. And I'd say that's a code review success. We stopped a bug getting into production. It's 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 PAP 8. 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 PAP 8. 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 8 is one of my favorite tools. It combines a lot of tools, and it enforces the Flake 8 guidelines, some of them as rules. And in this coverage.py, it checks your line coverage and requires.io, which checks for 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 8, which lints 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 just change 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 is 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 Comer. If I add a comma at the end of that line, then when I add a new item, I only have to see when I add a comma at the end of the silica, 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 going to 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 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 a 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 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 a bit of fun. They've got a high scoreboard. And 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 submit some code, 500 points if you close a ticket, but you actually get 1,000 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 having new code if the code that's already been submitted isn't merged. Hi. Great talk. 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? 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 going to 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, let's say, if it's Elasticsearch. Connect to Elasticsearch, 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 found a flaw in the test. 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? 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 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 merge that into master. Hi, Mikan. What about time spent? What do you suggest? You can go really in deep, like reviewing every line, texting or 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 going to miss bugs and stuff. But as for scrutiny, 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 trust. OK, 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 several issues, and all those issues can be resolved in maybe one or two lines of code. Do you favor 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 did that come up? That's the advantage 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 all.
 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  or all those other kind of things that you see on job posts.  And we should start trying to be code nurses and code tree  surgeons.  And I kind of see the reviewers 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 Gira, 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 app's 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 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 been 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 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 Bowink.  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 Alana.  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, they're conflicts with our goal  to share knowledge with the team.  And that might be OK, 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 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 ask that someone else looks at it.  And I'd say that the first step to getting better at review  and all those previous steps I 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, Eleanor 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 going to 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, Eleanor resubmits this code.  And I'd say that's a code review success.  We stopped a bug getting into production.  It's 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 PAP 8.  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 PAP 8.  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 8 is one of my favorite tools.  It combines a lot of tools, and it enforces the Flake 8  guidelines, some of them as rules.  And in this coverage.py, it checks your line coverage  and requires.io, which checks for 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 8, which lints 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 just  change 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 is 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 Comer.  If I add a comma at the end of that line, then when I add a  new item, I only have to see when I add a comma at the end  of the silica, 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 going to 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 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 a 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 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 a bit of fun.  They've got a high scoreboard.  And 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 submit some code, 500 points if you close a ticket,  but you actually get 1,000 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 having new code if the code that's  already been submitted isn't merged.  Hi.  Great talk.  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?  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 going to 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, let's say, if it's Elasticsearch.  Connect to Elasticsearch, 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 found a flaw in the test.  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?  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  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 merge that into master.  Hi, Mikan.  What about time spent?  What do you suggest?  You can go really in deep, like reviewing every line, texting  or 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 going to miss bugs and stuff.  But as for scrutiny, 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 trust.  OK, 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 several issues, and all those issues  can be resolved in maybe one or two lines of code.  Do you favor 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 did that come up?  That's the advantage 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 all.
Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_0_1175.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_1176_3539.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_3540_4907.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_4908_7559.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_7560_9287.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_9288_9671.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_9672_12419.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_12420_12515.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_12516_13883.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_13884_17375.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_17376_17579.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_17580_18275.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_18276_18755.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_18756_19151.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_19152_20135.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_20136_21971.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_21972_22199.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_22200_22547.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_22548_23135.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_23136_24227.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_24228_25151.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_25152_25319.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_25320_26099.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_26100_27935.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_27936_30527.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_30528_30587.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_30588_30659.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_30660_30899.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_30900_31703.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_31704_31883.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_31884_32099.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_32100_32279.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_32280_32471.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_32472_32555.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_32556_32939.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_32940_33095.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_33096_34127.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_34128_36767.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_36768_37355.png Adam_Dangoor_-_Another_pair_of_eyes__Reviewing_code_well_37356_37676.png
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 or all those other kind of things that you see on job posts. And we should start trying to be code nurses and code tree surgeons. And I kind of see the reviewers 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 Gira, 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 app's 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 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 been 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 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 Bowink. 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 Alana. 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, they're conflicts with our goal to share knowledge with the team. And that might be OK, 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 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 ask that someone else looks at it. And I'd say that the first step to getting better at review and all those previous steps I 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, Eleanor 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 going to 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, Eleanor resubmits this code. And I'd say that's a code review success. We stopped a bug getting into production. It's 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 PAP 8. 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 PAP 8. 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 8 is one of my favorite tools. It combines a lot of tools, and it enforces the Flake 8 guidelines, some of them as rules. And in this coverage.py, it checks your line coverage and requires.io, which checks for 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 8, which lints 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 just change 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 is 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 Comer. If I add a comma at the end of that line, then when I add a new item, I only have to see when I add a comma at the end of the silica, 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 going to 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 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 a 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 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 a bit of fun. They've got a high scoreboard. And 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 submit some code, 500 points if you close a ticket, but you actually get 1,000 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 having new code if the code that's already been submitted isn't merged. Hi. Great talk. 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? 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 going to 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, let's say, if it's Elasticsearch. Connect to Elasticsearch, 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 found a flaw in the test. 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? 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 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 merge that into master. Hi, Mikan. What about time spent? What do you suggest? You can go really in deep, like reviewing every line, texting or 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 going to miss bugs and stuff. But as for scrutiny, 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 trust. OK, 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 several issues, and all those issues can be resolved in maybe one or two lines of code. Do you favor 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 did that come up? That's the advantage 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 all.
Hello, everyone. I'm happy to introduce Adam pair of eyes reviewing code well. Thank you. I've become a much better programmer thanks to 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 taught. So today, I'm going to talk about these someone else's code and they look for things before merging code because we want to catch bugs better. And it's important to think about how we can use code review to improve ourselves and the this one is written by me. The name of the user's allow developers who are unfamiliar with the code passing, but sometimes we know that CI isn't That could be with the author or whoever's practices from blog posts, then search for blog lucky enough to be thrown in at the deep end of a Because as a reviewer, I need to consider the right now. Sometimes it's security. Sometimes performance issues. So I've kind of wasted some review. And you might be able to help decide effective and find more bugs so your software is them. And I find that teams which value a review because they have a backlog of code that's been you stand on this? So I don't always do it. flaw in the test. So I do get my editor out. features, and you put those branches into that programmer on your team, then maybe you want to but usually in my reviews, at least where I work different practice. Lots of people do, but I