 What is code review? I tend to start all my talks to the definition because that seems like a lot of fun for the audience. Code review is reading other people's code. If you're not looking at a patch file or a diff file, you're not doing code review. You might be reviewing something else and that might be something really valuable, like reviewing the architecture, capacity planning, something like that. That's fine, that's great, that's probably really valuable. It's not code review. And I'm only gonna talk about code review today. So why code review? It seems like a lot of effort, right? You have to write the patch and you have to get somebody else to review the patch. It's like twice as much work. You can't have your engineers getting half as much stuff done. It's a giant waste of money. Why would you bother? The first reason is to raise the bus factor. The bus factor, if you're not familiar with the term, is how many people would have to get hit by a bus before no one could work on your project anymore? So this really forces you to raise the bus factor because it forces another person to be familiar enough with your code to pass judgment on it. And for me, if I went before, I'm able to say, this code is really good or, hey, this code needs some work. That's a really high bar for me to be familiar enough to do that. And the more people who are able to understand that code at that level, the more people who'll be able to contribute to it and help maintain it going forward. Further, it helps ensure the readability of your code. Reading code and writing code are really two different skills. And for code to be readable and code to be writable are really different. Code is really writable when it's as short as possible. And those of you who've done Perl in the past life know that's a very different thing from code being readable. The person who writes the code is therefore probably not the person best able to judge if it's readable. And so forcing someone else to look at it helps control for that and optimize for readability. Third, it helps catch bugs. Somebody else reading your code, thinking about it from a different perspective and a different headspace with a different set of experiences and just being fresh with it makes it easier to find bugs. Other people will catch the things you missed. And finally, and I think most importantly, Code Review helps encourage a healthy engineering culture. People need feedback to get better at what they do. Code Review gives you a structure to have feedback. When feedback is irregular or disjoint and it's not a core part of your job, feedback is often I've seen used for criticism instead of as a way to help people grow. Pretty much everything I'm gonna talk about going forward works on two conditions. That people are acting in good faith and that people are committed to not being jerks and understanding that they're reviewing code and not people. These apply to both reviewers and people who are being reviewed. There are a lot of people who seem to believe that doing good Code Review requires being a jerk. This is totally not true and it's just an excuse to be mean and much rather people who thought that way would just walk away. So here are my ground rules for how to do really effective Code Review whether it's at your job or in an open source project. First, don't make people do things computers can do. People are really good at a lot of people things and very bad at machine things whereas machines are really good at machine things. Code Review does not mean downloading the patch, applying it and running the tests manually, looking for style guide violations, making sure the diff applies. Get a CI server to do those things. They can all be automated. Humans screw that stuff up all the time because it's boring, it's monotonous, it takes time and there's all sorts of ways you'll get it wrong. Plus, I found interestingly that for really boring feedback, like hey, these lines are too long, it violates our style guide this way, people hate getting feedback from the people. When people give that feedback, it sounds like, oh, this person's being a pain, why can't they just accept it? Like, it's just two characters over the limit. When machines give this feedback, it's like hey, computers, that's what computers do. They're pains and they're picky and they have no judgment. It makes it really easy to consistently enforce things in your style guide. Probably the one hard and fast rule I have that you cannot break is everybody gets code review. Code review is not a system that senior engineers apply to junior engineers or that old people apply to new people, it's something everybody does is a core part of your job. The person on their very first day gets to code review the very first engineer at the company. No one is above review and no change is above review. Changes where I was like, oh, this is so small, I don't need anybody to look at it. Have like a 100% rate of breaking the build. I think it's really important that code review happens before you merge a patch in. I've worked on some projects and I know some teams have a policy where you can actually get code reviewed after it's landed. I think this actually causes a lot of problem because it really promotes this feeling of inevitability. Oh, it's already landed. I don't want to cause trouble by like pointing out this stuff. It's probably not that important. I'll just let it slide. You don't give feedback that you would otherwise and then it's valuable to get all the feedback out. And again, every patch, every time, no patch is too small to be code reviewed. This also I found forces you to have a system where it's not such a pain in the neck to do a code review on the two line patch. You gotta have a system that's really easy so that you don't have a problem about every time and nobody, you never have to have an argument about what is and what isn't too small. Hopefully I've sold you at this point. How do you get started? First, get a tool to help you out with these. I've used Fabricator, GitHub, Ixerit, whatever tool, it doesn't matter. The only things that are important are the tool helps you keep track of patches and lets you leave comments and feedback on particular lines of code very easily. It doesn't matter which tool, any of those tools are fine as long as you have it and you find the tool is easy to work with with your team. So there are a few rules for what to do if you want to be a good patch author and make it easy if people are reviewing your code. First, give a description of what your patch does or what it's supposed to do at least. It's way harder for the reviewer when they have to reverse engineer what your patch does from reading the code and it makes it easier, makes it harder to find bugs if they have to reverse engineer it. If what your patch does doesn't match what it's supposed to do, it's a bug but you can't ever see that if you also have to figure out what it was supposed to do. Keep your patch small. A study IBM did shows that beyond about two to 400 lines of code, it's much, much harder for people to do good code reviews and find bugs. So it's super important to keep your patches small. As a reviewer, I would much, much rather a series of 10 200 line patches to review than one 2000 line patch. Somebody on Twitter joked that an engineer sees a two line change, they have 10 comments, 500 line change, eh, looks fine. Code review is a collaboration. You need to work with your reviewer to get your best possible patch. If you disagree with the reviewer, reviewer have a conversation with them. Maybe they misunderstood, maybe there's some way you can change the patch to have both of you agree on where it's supposed to be. So then there are a few rules on how to be a good reviewer. Starting with, what are you looking for? The first thing you're looking for in a patch is the intent. What is the patch supposed to do? And is what it's trying to do a bug? Do you really want the new feature this person is proposing? Maybe that feature's really out of scope for your project. Maybe the thing they think is a bug is actually a bug in the documentation where you misdescribed the feature. This is the second phase. Design is the change they're making in the right place. Did they change the CSS when really the HTML was wrong? Did they change the JavaScript when actually your API was sending back the wrong data? Looking at does the patch address the problem in the correct way? The next step is implementation. Does the patch actually do what it says? Is it complete? Does it have tasks? Does it add a new bug? Does it break an old feature? Is it documented? Does the documentation make sense? This is most of what you do. This is reading the patch line by line and figuring out is it doing things in the right way? Is this good code? Finally, the last step is what I call grammar. This is tiny stuff. Does the variable have a good name? Should something be a keyword argument instead of a positional argument? The little tiny stuff. Is there a typo in the doc string? Really small things. Finally, I think as you're reviewing a patch, it's really important to start at the top, looking at intent and look at the grammar in the last thing. Why? If you start a grammar, you get bogged down and looking at the typos and picking out the variable names. You lose sight of what you're reviewing. It's hard to make big global judgments when you're so stuck inside one tiny function and it's really, really hard to go backwards. So there are a few different types of elements, few different things you'll be pointing out as you go do a review and are leaving comments on the code. These are to-do. First are to-dos. These are the most important. These are things that must be fixed before a patch gets landed. Oh, I noticed this will break this other use case that's supposed to be working. Things like that. Next is a question. These are points for clarifications. Would it be faster if we rewrote it like this? Is there a library in the standard lib we could reuse for this logic? Nothing necessarily needs to be done. It's just a question that should hopefully provoke a conversation between the reviewer and the patch author. Finally, the last group are suggestions for follow-ups. These are things that don't belong in the patch itself, but the author might wanna do another subsequent patch. For example, when adding a new feature, you might wanna suggest that bugs get filed so that you can use that feature somewhere else in the code base later. It would really bloat the diff to change everywhere in the code base now, but you wanna make sure you go back and do it and maybe delete the legacy way of doing this later. I think code review is a super important part of engineering culture and a really, really necessary thing to help people grow in their jobs and have a culture where everybody works with everybody to produce the best patches. One of the really best things I've seen on some of the projects I've been doing every patch gets code review on is open source is that you can promote either a really individualistic thing where the patch reviewer is giving the patch author feedback and it's like the patch author's responsibility and it's like a really one-way communication or it can be this really multi-way dialogue where a whole group of people are providing feedback and trying to get the patch to be as good as it can be and this promotes a really group ownership of the code. I think it's the best possible thing. Code in a big repo should never be owned by one person. It's collectively owned by everyone who's responsible for it and code review can really help you maintain this. Thank you very much.