Phil Booth

Existing by coincidence, programming deliberately

The art of good code review

This post is an extension to a talk I gave recently at work. It was arranged at short notice and the audience were experienced engineers, so I kept it brief and tried not to be patronising. But the feedback afterwards was quite positive and there were some questions too, so here’s the extended version for anyone interested. Note that our team conducts pre-merge reviews, and some of the suggestions are specific to that context. I actually prefer post-merge reviews, but am yet to persuade everyone I work with that they’re better. Oh, and trigger warning: this post contains opinions.

Why do we review code?

First, let’s establish what the point of code review is and also what it isn’t.

The number one, most important reason to review code is shared ownership. “Ownership” can be tricky to define in code terms, but mostly it’s a feeling. It means you understand the code, that you feel empowered to change it and the responsibility to maintain it. It doesn’t necessarily mean that you wrote it.

Shared ownership is important because individuals fail in lots of ways. They get sick or move jobs or get promoted. And code fails too of course, so whenever important code is owned by an individual, that’s a timebomb counting down to sabotage your organisation (usually late on a Friday night). Reviewing code (along with other techniques, like pair programming) is a way to dismantle knowledge siloes and reduce the likelihood of emergency situations occurring.

The second most important reason we review code is “the big picture”. Engineers are naturally detail-oriented people. The job literally forces you to focus on details in order to do it well. When you spend most of your day thinking about and working on small details, it’s very easy to lose sight of broader concerns. A code review is an opportunity to address that, to step back and consider the broader view. Does that small component you’ve been building in isolation make sense in the context of the wider system? Does it compromise security or observability or performance? Is it robust? Are there simpler approaches?

Trailing far behind those two are a host of less important reasons for reviewing code: improving code quality, upholding conventions, correct formatting, catching bugs, spotting gaps in test coverage, eliminating dead code, etc. Those things are all fine, but they’re less important because code review is an unreliable way to catch them. Some things will get spotted but many won’t. If code review is your only mechanism for fixing them, you are absolutely guaranteed to fail. That’s why we have tools like linters, compilers, type checkers, formatters and various flavours of automated testing. If no tool exists for a thing you want to check, write one that does. And when machines are doing that work for you, your humans will be happier too.

What does a bad code review look like?

lgtm 👍

Not all “lgtm” code reviews are bad, of course. Some changes are trivial or just obviously correct, and “lgtm” is fine on those. But for more complex changes, if the only comment is “lgtm” it can be a sign the reviewer hasn’t properly tried to understand it. And understanding is the primary aim, because of shared ownership. Code review is not a rubber stamping exercise.

Another sign of bad code review is when reviews show up too quickly. There’s a lower bound on how fast it’s possible to read code and any faster than that should be an alarm bell. Although again, there can be exceptions. Perhaps a change was worked on in partnership and the reviewer is already familiar with it.

Bad code reviews can also happen when lots of discussion is generated on trivial changes, but the complicated parts sail through without any comment at all. This is bikeshed territory and it can escalate to poisonous levels if not corrected.

Good code review requires a good PR

The burden for a good code review does not rest solely on the reviewer. Initial responsibility falls on the reviewee to make their changes easy to understand.

That starts with the description, which should be clear, detailed and precise. It should introduce the change and include all of the context required to understand it. If the context is too long to describe in detail, summarise it and link to the original sources of information. The description should cover what the change is, why it’s needed and how you’ve approached it. If there are approaches you opted against, mention those too and why you decided against them.

Then the change itself should only contain the minimal diff necessary to implement the description. If there are tangential refactorings, extract them to a fresh branch and get them reviewed separately, in advance of the main change. Avoid frivolous changes that don’t serve to improve the thing you’re working on. Every single part of the diff should have a purpose that you can justify when asked to do so.

You should make sure all automated checks are green before asking for review. A reviewer’s attention is a finite resource, don’t waste it on things that aren’t finished yet.

All behavioural changes should come with equivalent modifications to the tests. New features should have new tests, behaviour modifications should modify existing tests. If there are no existing tests to modify, take the opportunity to write some new ones so that nobody can accidentally break your change in future.

If the change itself was automated, include sufficient information for a reviewer to review the invocation instead of thousands of lines of generated output. They’ll want to eyeball the output too of course, but the command you used or script you wrote is the real thing being reviewed here.

Lastly, before asking others to review, conduct a self-review yourself first. It doesn’t take long and you might be surprised how many mistakes it catches. Typos, leftover TODO comments, debug logging, skipped tests, commented-out code etc. You’ll appear far more competent to your peers if that stuff doesn’t show up in your pull requests.

What does a good code review look like?

There are five things you want to get right as a reviewer.

  1. The description.

    Before you even get to looking at the code, read the description. Is there one? Does it make sense? Does it include all of the information required to understand the code?

    This is not pedantry. You need all of that sorted out first, to be certain you understand what you’re meant to be reviewing. If you’re consistent about it, engineers will quickly adapt to your expectations and writing good descriptions will become a habit.

  2. The code.

    Reading the code means more than just glancing at its outline. You need to actually read every token on every line. It’s supposed to take time. You can’t read a 500-line diff in 5 minutes, so carve out enough time in advance that you won’t feel rushed to finish and move on to the next thing.

    You don’t have to understand everything straight away. Ask questions about parts that aren’t clear. Don’t worry about sounding stupid, nobody is going to think that and the fastest way to be right about something is to be wrong about it in public. For parts you think you understand, check your understanding by writing it down and asking the reviewee to confirm it (or correct you if it’s wrong). If you see something that looks funny or weird or surprising or complicated, mention it.

    Gradually build a mental model of the code, then ask yourself the key question that underpins the concept of shared ownership:

    “Am I happy to maintain this?”

  3. The tests.

    Tests are just another part of the code, but I’m calling them out separately because sometimes people completely skip over them when reviewing. Understanding the tests is as important as understanding the code. Tests that are hard to understand are less likely to be maintained in future. And good tests can double as helpful documentation for what a system can and can’t do.

    Make sure the things that should be tested are actually being tested. This is not just about coverage, you need to look at the assertions too. 100% coverage is meaningless if assertions don’t enforce the right invariants.

    While reviewing the tests it can also be helpful to go back and review the relevant code again. Compare the two side-by-side and look for things that seem missing or out of place. Ask questions about anything that doesn’t match your expectations.

  4. Commenting.

    There are three rules for discussion in code review: be considerate, be honest and be open-minded.

    Be considerate because there are humans with feelings at the other end of the review. Teams work much better when there’s mutual respect and everyone gets along. So any criticism should be of the code, not the person. It should be made dispassionately and come with reasoning attached. If possible, alternative suggestions should be mentioned too, although avoid being over-prescriptive and try to leave the reviewee some space to figure out their own solution. Don’t suck the joy out of programming.

    Be honest because code review is a way to improve practices across the team. It fails at that objective if you withhold opinion in a misguided attempt to be “nice”. Honesty will also lead to your own mistakes being corrected occasionally, which means you benefit from it personally too.

    Be open-minded because mistakes are inevitable on all sides, including your own. Don’t cleave to your opinions too tightly and take time to consider other points of view. If you find yourself stuck in a disagreement where neither side wants to budge, yield. It’s okay to disagree and move on, code review is not a competitive sport. If you really feel strongly about something, you can present your ideas as follow-up suggestions. Codebases are rarely finished, there’s usually an opportunity to improve things further down the line.

  5. Approval.

    Given that the main point of code review is shared ownership, it follows that you can’t approve a change before you understand it. Everything else runs secondary to that, so withhold approval until you’re confident that you fully understand the change.

    The flipside of this principle is that it’s okay to approve changes you disagree with, provided you understand them. Code review is not meant to be a dictatorship.

    If you’ve left comments about minor corrections, pre-approve the change anyway before they land. Your peers should be trusted to handle them appropriately, or even if they get it wrong that can be fixed later. Pre-approval ensures nobody gets blocked from merging the revised changes when they’re ready.

Code review checklist

What kinds of thing should you be looking for in code review? People sometimes pay close attention to a few pet areas and let other stuff slide. If that sounds like you, you can use this (incomplete) checklist as inspiration:

If anything above is something you’re not sure about, spend a bit of time learning around the topic then start by asking questions about it during code review. You’ll quickly level up in that area.