Some thoughts on code review

The following is a lightly edited version of an email I sent my team about code review. We use git pull requests, but most of what I write applies to any form of code review, especially offline (as opposed to in-person) pre-commit review.

Being a code reviewer is a duty and a trust. When you hit “approve” on a pull request, you are making a declaration that you are comfortable that the code belongs in your codebase in its current form. If there is anything about the request that makes you uncomfortable (e.g. no unit tests!), you are duty-bound to raise that concern. That doesn’t mean being unnecessarily nitpicky or combative, but it does mean that you shouldn’t avoid raising a concern out of expediency or because you think the feedback won’t be well received. More eyes are always better, and there is precious little code that can’t be improved by having another person look over it and make suggestions. As a reviewer, this is your job.

For this to work, it is important that everybody also receives review feedback in a spirit of goodwill. When your reviewer comes back to you with feedback or requests for changes, they are not doing this to attack you or shame you or stand in the way of you getting your work done. They are doing it to fulfil their duty as a reviewer. If you have a problem with the way your code in being critiqued, please don’t make your reviewer uncomfortable. You are welcome to come to me to discuss it, but I am likely to give them a fair bit of benefit of the doubt. Understanding that code criticism is not personal criticism forms a key part of egoless programming.

I know the concept of duty is an old-fashioned one, but I think it captures the act of code reviewing perfectly. Reviewing code is not always going to be fun or convenient, but if done correctly it can be the key tool to ensuring we have a codebase that we can be proud of. Great code is everybody’s responsibility, and it can only happen if the whole team works together to uphold high standards.

Here are some tips for making your code easy to review:

-Descriptive titles and code descriptions. I think we’re most of the way there, thanks everybody!

-Small, atomic changes. This makes it much quicker and easier for a reviewer to understand (and hence get comfortable) with a change. It’s possible (and in many cases preferable) for a single PBI to generate multiple PRs. Atomic means not able to be broken down further: each PR should do a small number of self-contained things, and subsequent PRs can build on earlier ones. Git makes this pretty easy. Tip: If it takes more than a couple of sentences to describe your PR, consider whether it is truly atomic or should be broken down further.

-Tests. One of the many wonderful things about tests is that they act as a form of documentation. Adding tests to your PR will make your change much easier to review.

And here are some tips for being a good reviewer:

-Quick turnarounds. You don’t have to drop everything when a PR comes in, but don’t keep the requestor hanging for any longer than necessary.

-Minimise round-trips. That means getting all your feedback out in a single pass rather than making people go through multiple rounds.

-Meaningful comments. If you want the requestor to change something make it clear what, and why.

-Be respectful. Don’t use emotive language (e.g. “this is a dumb way to do it”) and remember that it is your job to help the requestor submit high quality code that does what they need it to do. I find the Socratic Method works pretty well to keep things respectful (e.g. “why did you do it this way?”).