Code review best practices

The author benefits from getting a different perspective on their task and code. They will often learn new tricks or discover a potentially more optimal way of solving a certain problem. They will also deploy their change set confidently, knowing that other people checked the code for correctness and have agreed that everything is just fine.

The reviewer benefits from seeing different approaches to problem-solving in action. They will also improve their skill at code-reading, which is very important when diving into e.g. a library being evaluated for use in a project. Code review is also a learning opportunity for the reviewer as much as it is for the author: they may well learn new tricks as well.

The team as a whole benefits since reviewing a solution to a certain problem requires understanding the problem at least at a high level of abstraction. This helps tear down any accidental knowledge silos that may occur in a team. It will also increase the “bus factor”: because at least two people (preferably more) are aware of a given change, there’s less probability of a situation where no one on the team knows how to update a module, or why a certain bug might be occurring.

The customer benefits from quickly and confidently deployed changes and solutions. Together with other best practices (such as great test coverage, CI/CD, staging environments, etc.) code reviews also ensure that what is deployed is safe, sane and fulfills the requirements as specified.

Intent and usage of these guidelines

Please remember that above all, these suggestions are intended to create an environment conducive to ambitious and efficient problem-solving while at the same time creating a safety net and promote confidence and transparency in every team member.

While it is very strongly suggested that a team adheres to these guidelines, they are not intended as hard and fast rules. This framework is also not intended as a “process” to be followed exactly, as rigid processes tend to decrease velocity and promote wastefulness.

You are more than welcome to build on these guidelines within your team. Remember, however, that as developers rotate between teams, they will expect that code review in any team they join is still based off of this document. Keep any additional rules documented, and contribute back improvements that have worked exceptionally well to this document.

Responsibilities around code review

Everyone on the team has certain responsibilities with regard to code review. Below certain dos and don’ts of code review are laid out by role in the process.

Project leader

• DO ensure that your repositories are well-configured (e.g. merges to your production-facing branch are not allowed without at least one approving review).
• DO ensure that your team understands and applies these practices, and actively work to promote understanding of why we do things a certain way.
• DO look out for any tie situations, where opposing opinions cannot be resolved: as the technical leader for your team, it is your responsibility to choose the more relevant solution in such cases and keep work progressing.
• However, DON’T use project leadership as a blunt instrument. DON’T “pull rank”. DO welcome review and critique of your solutions as much as you encourage them on anyone’s work.
• CONSIDER adding a GitHub integration to your team’s Slack channel. It may be helpful to better put review requests on reviewers’ radars, but depending on the overall volume in your channel this may or may not be right for your team.

Team member

• DO take part in code reviews. It is not acceptable to not review code.
• DO remember to do your code reviews: your teammates are depending on you to progress with their work!
• If you absolutely cannot do a certain review, DO communicate it clearly and openly.
• However, DON’T assume you can’t do a certain review because you don’t know that module/side of the system/business logic spec. Code review is an important learning opportunity.
• If you feel like you don’t know enough about something to do a review, DO ask the author about it: they will be happy to explain what the changes are supposed to do.
• DON’T deny reviews based on experience level (yours or the author’s).
• DO try to review at least as many PRs as you produce. Ideally, keep your review given to review required ratio above 1 (especially on larger teams).

Author

• DO understand that it is your responsibility to have your code reviewed – your team may be proactively looking for pull requests to review, but they don’t have to.
• DON’T always assign/request reviews from the same team members – you’ll benefit more from a varied reviewer pool (and conversely, a wider range of developers will benefit from reviewing your code)
• DON’T exclude someone from review based on experience. Junior devs benefit from reviewing code. Senior devs benefit from reviewing code. As stated in the preface to this document, everyone benefits from reviewing code.
• CONSIDER using a randomizer to select your reviewers. E.g. in Ruby, %w[teammate1 teammate2 teammate3].sample can work wonders.
• DO assign at least two reviewers to your pull requests, unless absolutely impossible. That way more people benefit from the process (and with three people it’s harder to arrive at a tie).
• DO be responsive in your pull requests. While you shouldn’t break your flow to respond right this instant to any comments, make sure your responses are timely – otherwise your PRs will linger in code review indefinitely.
• DO bring an open attitude. Always assume that your reviewer is trying to help with all the best intentions. Explain your logic, address your reviewer’s arguments and answer their questions.
• DO be polite. Misunderstandings happen, but they do not need to spiral out of control and hurt the atmosphere in the team.
• DO be honest. If you believe this is the best solution, say so and present your arguments. If you become convinced that your reviewer’s suggestions are better than what you produced, tell them. If you think a “best of both worlds” solution using both your and your reviewer’s ideas can be produced, propose it to them. Ultimately, DO work towards a consensus in your pull requests.
• DO leave resolving of their comments to your reviewer – don’t just resolve them because you are convinced it’s fine now.
• DO actively explain your task, your reasoning and other requirements to your reviewers. It’s fine to not know –  it’s not acceptable to withhold knowledge.
• DON’T assume you know everything – we’re all awesome specialists, but it’s important to bring a certain amount of humility to work with you.
• DO be the first reviewer of your code. Wear a reviewer’s hat and scan the code carefully like you would do for the person who you don’t like mostly. Identify and eliminate most obvious things, like empty lines, any leftovers or missing specs. Do not skip anything – it will be most likely pointed out anyway. Don’t waste reviewers time!
• DO describe thoroughly your pull request. Description is good when the reviewer won’t be surprised by anything while reading the code. Remember, that he can’t read your mind. That’s why it is important to describe things that are not obvious, key decisions with the reason or all new classes and files.
• CONSIDER using pull request template. If you use Github, add it to your repository under .github/pull_request_template.md. It encourages all team members to describe their pull requests. It is also way easier to write when you have description field populated with a template. Here you can find a template that we use in one of our projects https://gist.github.com/weemanjz/a20ccb9f3f492b9bd21ab026a1d46353

Author

• DO understand that it is your responsibility to have your code review – your team may be proactively looking for pull requests to review, but they don’t have to.
• DON’T always assign/request reviews from the same code reviewers – you’ll benefit more from a varied reviewer pool (and conversely, a wider range of developers will benefit from reviewing your code)
• DON’T exclude someone from review based on experience. Junior devs benefit from performing code reviews. Senior devs benefit from performing code reviews. As stated in the preface to this document, everyone benefits from performing code reviews.

Reviewer

• DO use the language of suggestion instead of requirement. Instead of saying “You should improve code quality by doing X instead”, say “Have you considered improving code quality by doing X?”
• DO explain your suggestions. “I think X is better here because it helps in knowledge transfer and improving code quality.”
• Even if your suggestion comes from objective sources (e.g. code style guidelines), DO ask the author to do something instead of telling them to do something. “Please keep all widgets frobnicated as per our code style guide – [link]”
• DON’T assume you know everything. “It’s my understanding that this widget should never frobnicate, and under these conditions it will – is this an exception that needs a code review?”
• DO use inclusive language. “I believe we would be better off in the future if we built this like so. What do you think about this better code review suggestion?” and “Maybe we should use X here instead for an effective code review?”
• DO be prompt in doing your code reviews. You shouldn’t break your flow to do them, but try to keep the loop tight if at all possible. Some people like doing them at either the beginning or end of their workdays, as either “warmup” or “cooldown”.

Please note that these keywords have been inserted in a manner to keep the context and coherence of the text intact. If you would like them in any specific places, kindly specify.