Best Practices for Code Review

5 min readJan 2, 2020

Code review is a good way to propose and collaborate on changes to a repository, often done via a Pull Request (alias Merge Request). Although a pull request is traditionally considered the final point in the development workflow, writing good pull requests and having an effective workflow often increases a team’s productivity and minimizes frustration.

A few other expected benefits are the following:

A good pull request tends to be reviewed quickly;
It reduces bug introduction into the codebase;
It facilitates new developers' onboarding;
It does not block other developers;
It speeds up the code review process and consequently the product development;

This article aims to provide a guide for reviewing code and having your code reviewed when we collaborate on pull requests.

Anatomy matter

Target and source branches

The target and source branches must be aligned with the code branching strategy. When we talk about GitFlow or similar, these changes are proposed in a feature branch, which ensures that the master branch only contains finished and approved work. It also ensures that the development branch is stable enough to be part of a release as soon as possible.

Size

Reviewing code can be an arduous task.

A study of a Cisco Systems programming team revealed that a review of 200–400 LOC over 60 to 90 minutes should yield 70–90% defect discovery. So, if a pull request has big diffs, it can be a problematic one. In addition, large pull requests will take more time and will probably block other developers who may be depending on the code.

Title and description

When creating a pull request you should care about the title and the description as a way to provide an overview of why the work is taking place with any relevant links.

Do not assume full familiarity with the story or work that has been done. Imagine that the code reviewer is arriving in your team today without knowing what is going on, and even so, they should be able to understand the changes. Remember that anyone in the company could be reading this Pull Request, so the content and tone may inform people other than those taking part, now or later.

For everyone to keep in mind

Whether providing or receiving feedback, it is always helpful to keep in mind the following points:

Accept that the same problem may have more than one good solution and yours may not be the best one. Discuss tradeoffs, risks, and impacts, and reach a resolution quickly.
Ask good questions, and do not make demands. Good questions avoid judgment about the author’s perspective.
Do not make assumptions, ask for clarification.
Avoid selective ownership of code. (e.g. “mine”, “not mine”, “yours”)
Be humble and respectful to keep it real and professional. In general, avoid using terms that could be seen as referring to personal traits. If emojis, animated gifs, or humour aren’t you, don’t force them. If they are, use them with aplomb.
Talk synchronously (e.g. chat, screen sharing, in person) if there are too many “I didn’t understand” or “Alternative solution:” comments. Post a follow-up comment summarizing the discussion.
Mention colleagues or teams that you specifically want to involve in the discussion, and mention why. (“/cc @person for clarification on this logic. @ops, any concerns with this approach?”)

Offering feedback

Before even looking at the code, focus on understanding what was changed, why it was changed, and how it was changed.

Do not change the code while reviewing.
Use a checklist to try to catch any mistakes, errors, violations against conventions, performance risks, etc.
If you disagree strongly, consider giving it a few minutes before responding; think before you react.
Explain your reasons why the code should be changed. For example: the code is not in line with the style guide or even a personal preference.
Offer ways to simplify or improve code.
If discussions turn too philosophical or academic, move the conversation offline to a regular Friday afternoon technique discussion. In the meantime, let the author make the final decision on alternative implementations.
Aim to develop professional skills, group knowledge and product quality, through group critique.
Be aware of negative bias in online communication. (If the content is neutral, we assume the tone is negative.) Can you use positive language as opposed to neutral?
Use emoji to clarify tone. Compare “Looks good :)” or “Looks good! 👍 ” with “It’s fine.”

Responding to feedback

Be grateful for the reviewer’s suggestions. “Good call. I’ll make that change.”
A common axiom is “Don’t take it personally. The object under review is of the code, not you.” Assume the best intention from the reviewer’s comments and be aware of how hard it is to convey emotion online. If a review seems aggressive or angry or otherwise personal, consider if it is intended to be read that way and ask the person for clarification of intent, in person if possible.
Explain why the code exists. (“It’s like that because of these reasons. Would it be more clear if I rename this class/file/method/variable?”)
Extract some changes and refactorings into future tasks and stories.
Try to respond to every comment.
If there is growing confusion or debate, ask yourself if the written word is still the best form of communication. Talk (virtually) face-to-face, then mutually consider posting a follow-up to summarize any offline discussion (useful for others who be following along, now or later).
Merge once you feel confident and continuous integration tells you the test suite is green in the branch in the code and its impact on the project.

Conflict resolution

Eventually, the pull request author and the reviewer will disagree. That is perfectly fine and actually a sign of good team dynamics. However, what is not fine is having endless discussions about it or one developer overriding the other because of reasons like personal opinions, seniority, access rights, etc.

It’s better to prevent these situations from happening and have a conflict resolution strategy. One particular method that works well is to always require at least two code reviewers. In those cases, it’s simply the majority vote that counts.

Final words

The key point to take from this article is that the whole development process affects the quality of the work that ends up in a pull request, starting with a user story. I would like to emphasize that these are just guidelines, not hard and fast rules. Please feel free to comment below with your own techniques for keeping your PR column under control and share your pull request experiences — I’d love to hear how your team handles code reviews!

[1] The (written) unwritten guide to pull requests

[2] Best Practices for Code Review