How to review code

Tips for having meaningful and smooth code reviews
A code review that doesn’t go right. (image by Manu Cornet, CC BY-NC-ND 3.0)

A code review that doesn’t go right. (image by Manu Cornet, CC BY-NC-ND 3.0)

I’ve reviewed over 3000 code changes so far in my career, and gone through about same amount of reviews as change author. When reviewing code, here is the golden rule I follow:

Any code change that’s a strict improvement should be approved.

Most incoming code reviews are both improvements in some aspects and make things worse in other aspects.

Understand the change

It’s tempting to glance over a change and only comment on small technical details or style issues, but this will eventually lead to worse code. In order to give meaningful feedback, understand the change to the extent where you could have authored it yourself.

Blocking issues to look out for

  1. New code is unclear. Ask: “I do not understand this”. Insist that it’s clarified in the final submitted code rather than in a review comment.

  2. New code is missing tests. Insist.

  3. New code does not fit into surrounding code in terms of style or common libraries used. Insist. Link to a canonical style guide whenever you can.

  4. Pre-existing tests were changed willy-nilly until they run green. This often takes the form of only a few lines added to every existing test case. These drive-by fixes accumulate easily over the course of some changes, until the core idea behind the test cases is obstructed. Insist on refactoring the tests.

  5. New code introduces duplication with existing code. Ask to deduplicate. (This is particularly often seen in tests.)

  6. The change is increasing code complexity.

    • Is there a better solution you can suggest?
    • Does the change’s benefit outweigh the additional complexity?

  7. The change introduces dependencies in places where they don’t belong.

Non-blocking issues to look out for

It’s fine to ask for additional changes that are not strictly required, but always make it clear that they are optional.

  1. “I would have done it differently." is a gut reaction almost all reviewers have sometimes. But it’s a shortcut heuristic the mind takes which usually has a more solid technical background.

    Learn the specific technical background to your gut reactions, so you can give concrete advice.
    If you can’t find a specific technical problem, consider asking why it was done in that particular way. There are often be good reasons which only need to be clarified a bit in the code.

  2. Pre-existing issues. Point out if pre-existing issues can be fixed in the same change, but explicitly mark it as optional. Even when not done immediately, this discussion helps to form a common understanding which pre-existing patterns in the code should be avoided and kept.

Taking technical debt: A game of trust

Sometimes you will want to approve newly introduced issues and postpone their fixes until later (Technical debt). Insist that the author will fix these issues after the submission.

In these cases, code reviews are a game of trust. A strategy that works for me is: Assume the best from people you don’t know yet, but don’t let them pass a second time if they didn’t pay off the technical debt.

Good ways to track technical debt issues are (1) a good memory for names, (2) a shared bug tracker and (3) TODO markers in the code (for smaller things).

Behaviors to avoid as a reviewer

Parting words

I hope this collection of code reviewing tips will help someone to have more constructive code reviews. Remember:

The reviewer’s job is not to stand in the way, but to enable the code submitter to do their best work.

Feedback is always appreciated. :)

Update 2019-09-05: My colleague Max Kanat-Alexander has compiled and open sourced Google’s guidelines for code reviews at https://google.github.io/eng-practices/. It’s a longer document compiled from the input of many engineers and has a lot of good advice. I can wholeheartedly endorse this.

Comments