![](/images/2011.09.18_code_reviews.png)
A code review that doesn't go right. (image by [Manu Cornet](http://www.bonkersworld.net), 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.
{.info}

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.

1. **New code is missing tests.** Insist.

1. **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.

1. **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.

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

1. **The change is increasing code complexity.**
   * Is there a better solution you can suggest?
   * Does the change's benefit outweigh the additional complexity?

1. **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.
   {.info}
   
   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](https://en.m.wikipedia.org/wiki/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

* **Do not sit on a code review.** Even if you can't do the review,
  explain immediately why you can't do it (yet) and redirect to
  someone better suited if possible.

* **Never insist that reviewees fix pre-existing issues.** Being code
  reviewer is a position of power, but it's not a free pass to
  blackmail others into doing your work.

* **Don't block legitimate changes for corporate politics reasons.**
  This should go without saying.  Do not use code reviews to unjustly
  influence project priorities.

## 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.
{.info}

Feedback is always appreciated. :)

> **Update 2019-09-05:**
> My colleague [Max Kanat-Alexander](https://twitter.com/mkanat) 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.
{.info}