More Effective Code Reviews
Derek Prior is a Developer at , where they have a strong code review culture. In this interview, Derek shares the benefits of code reviews and the essential elements he thinks you need to make them effective.
Read more interviews with some of the world’s best developers on * or follow us on or follow us on .*
When it comes to the benefits of code review “everybody’s natural reaction is to say that they catch bugs. That is true. If I have code reviewed that’s going to have fewer bugs than code that isn’t reviewed. But I think that that puts too much importance on the ‘finding bugs’ part,” says Prior. “It’s much more helpful to focus on the cultural benefits of code reviews,” which for him are many. From “sharing knowledge with each other, or keeping up with what everybody’s doing” to “knowing what’s going on in that other part of the code base and finding a really interesting alternative solution to a problem that they hadn’t thought of.”
For Prior, the benefits of Code Review go way beyond simply defect finding — “finding defects is actually, frankly, really hard. A lot of times I’ll talk to people about code reviews, and they’ll say, ‘well we did code reviews, but we still had all these bugs’. But they’re not going to catch all the bugs. You’re looking at the diff, and to know exactly how that’s really going to impact your system, you have to know the entire system”. So whilst “code review is great for defect finding, it’s not a panacea. Instead, I think it’s much more helpful to focus on the cultural benefits of code reviews”, says Prior.
One way to make sure that once you get started with code reviews, you stick with them is to “take a really, lightweight approach to it,” recommends Prior. For instance, “when I finish up a PR, I will paste it into Slack, or whatever. We’ll paste that in and say, ‘Can I get a review for this please?’ and generally, that’s enough, with the way we work, to get your code reviewed within the next few hours,” Prior says.
A fast turn around on code reviews is important, and it doesn’t have to be burdensome to stay on top of them. Prior suggests that there are in fact “a lot of natural breaks throughout your day that you can work these in”. “For me, I come in in the morning” and catch up on any code reviews from overnight. “Then, right before lunch, or in the afternoon, if I’m going to take a coffee break… I’ll look then”. “There’s plenty of times I find, that I can just work these in naturally. There’s no need to schedule them. I’ve worked with teams that try to schedule them, or try to say, ‘This person is going to be the one, that’s going to be chiefly doing code reviews this week’, and I’ve never seen that work particularly well”.
Instead, you just need to “keep it lightweight and friendly,” says Prior. “The biggest thing that makes this work is, keeping small, discrete pull requests, that are going to be much more easily reviewed if you provide some excellent context. That’s the ‘why’ you’re making this change, not necessarily the ‘what’”. But “ultimately, the big secret to this”, confesses Prior, “is that most of these code reviews only take me five minutes. It’s not a big commitment.”
“Part of what I think makes code review so great, is that it’s a great place to have a technical discussion about your actual software. Rather than in the abstract,” says Prior. He doesn’t subscribe to the idea of using a checklist or specific items, suggesting a more relaxed approach — “just look at things that interest you about a change”. “Maybe you just got finished reading this great book on design patterns. That’s valuable. You’re going to teach somebody something. Or maybe you’re really interested in web security… or accessibility.” For me, says Prior “I harp on naming a lot… I look for test coverage.” Whatever it is, so long as you have “a good blend on your team, of people who are interested in different things” then you “can all learn from each other.”
“The number one thing is context,” says Prior. “When you’re submitting a pull request, you’ve been working on this thing for four hours, or eight hours, or two days, or sometimes even a week, or whatever the case may be. You have a lot of context built up in your head. Some things seem really obvious to you at this point”. The thing to remember though is that “to the person reviewing it, they weren’t there… they don’t have that context”. “It really helps if you’ve been making several small commits along the way, where you’re describing what was in your head,” but regardless, “as you prepare the change… make sure you give a nice description of everything.”
This description should answer the question “Why are we changing it?,” says Prior. Other considerations are “why is this the best solution? What other solutions did you consider? What problems did you run into? Is there an area of the code you’re really unsure about?” These will all “help you get a much better review, by setting up everybody else to be on the same page as you”, Prior says.
Prior warns that “when code reviews aren’t done particularly well, or are overly critical, they can lead to resentment among the team”. One thing that you must be aware of though is that written communication is prone to negativity bias, says Prior. “If I’m talking to you and I give you some technical feedback, and I say, ‘Oh, why didn’t you use this pattern here’, you’re going to perceive that in one way. But if I say that same exact thing written down, it comes off as more harsh. You’re going to perceive that more negatively. It’s much more subject to your particular mood. I can’t influence it with the way I say something. It’s just a fact that, the same feedback written, is going to be perceived more negatively”. So this is something that should be considered in code review too.
Prior recommends an excellent way around this is to “give feedback in a manner that’s more of a conversation. What I like to do is ask questions… Instead of saying, ‘extract the service object here to reduce some of this duplication’, I would say, ‘hey, what do you think about extracting a service object here, to eliminate this duplication?’ They’re very similar comments, but now I’m opening it up to a conversation, by asking you a question”. Another suggestion Prior makes is “clarifying how strongly you feel about a piece of feedback.”
Nevertheless, from time to time conflict will still occur. But for Prior, “having conflict in your code reviews like this, is actually really beneficial… as long as they’re the right types of conflict and nobody feels bad about them afterwards. If you’re agreeing with your teammates all the time, and nothing interesting is happening in your code reviews, you basically have a monoculture. You want everybody bringing their own experiences, and their own expertise, and sometimes those are going to clash with each other”. But if you can “handle those conflicts properly”, says Prior, then that’s “how everybody on the team is going to learn.”
Read more interviews with some of the world’s best developers on * or follow us on or follow us on .*