Guidelines for the reviewer(s)

A code review is a software quality assurance activity where several people view, read the source code, and give feedback. The people performing the checking, excluding the author, are called reviewers. They are responsible for providing feedback and approvals to the code authors. Doing so requires being careful, thorough, and responsible.

What to look for in a code review

The code review process aims to ensure that the delivered code meets the story’s Acceptance Criteria (AC) and that the codebase stays consistent, maintainable, and understandable. The goal is not to find the “perfect” code; there is only better code. The reviewer should balance the need to progress forward with the importance of the suggested changes at any given time.

Functionality

Code reviewers must ensure the pull request (PR) passes all AC for a story. This check comes in addition to the functional testing done by the code author, i.e., the code author must not rely on code reviewers to catch basic functional issues on their behalf.

Code reviewers should focus their attention on the following:

• Edge cases or bugs that the code author might not have considered.
• Performance and security concerns.
• Putting themselves in the user’s shoes (i.e., thinking of the User Experience).

Conventions

While there are no magic rules, developers must ensure they pick good names for everything:

• A good name must be descriptive enough to fully communicate what the item is or does without being so long that it becomes hard to read. Very concise names without meaning, such as (‘a’, ‘i’, etc.) must be avoided.
• Naming must follow the standard names on a specific stack.

Ensure the code conventions on Compass for each programming language is followed.

Testing

Testing is required during development. So code reviewers must ensure author writes tests to cover their implementation code and that all added/modified tests are well-designed.

Positive areas

While code reviewers tend to focus on areas of improvement, they should offer encouragement and appreciation for good implementation and practices, e.g.:

• The implementation is well described in the pull request description, making the review a breeze.
• The code author has thought of edge cases that the code reviewer did not know about.
• The code author has used practical algorithms that could improve complex logic effectively.

Processing a pull request

Navigating a pull request in review

Take in the PR as a whole

Looking at the pull request’s title and description and what the PR does in general. Does it follow how to Open a good pull request guidelines?

• Title: [<User Story ID>] <User Story Title>
• Description: follows PULL_REQUEST_TEMPLATE

Check the main parts of the PR

• Looking at the story/issue provided in the first line of the PR’s description to get all the AC, we have an idea of what the change should be doing.
• Do What happened? and Insights provide enough information or a good approach that covers the AC?
• Does Proof of Work provide evidence of the working feature? Does it match the design, feature flow, or content?
• Looking at the Files changed, does the code have any major issues with code architecture, UI/UX problems, or affect any legacy features?

Build and Test

Continuous Integration (CI) ensures the PR is built and tested successfully before manual review. So the reviewer needs to build and check all or a couple of these expectations:

• The implemented UI looks like the design.
• The feature works correctly.
• The edge cases are correctly handled.

Speed of code review

Be aware of the speed

When code reviews are slow, several things happen:

• The velocity of the team as a whole is decreased.
• Code health can be impacted.

When reviewers are slow, there is increased pressure on both:

• Code reviewer: give review depth/quality that is not as detailed/good as it could be.
• Author: allow the author(s) to submit PRs that are not as good as they could be.

How fast should code review be?

• Reviewers should do a code review shortly after it comes in if they are not in the middle of a focused task.
• One business day is the maximum time it should take to respond to a code review request (i.e., the first thing the next morning).
• Following these guidelines means that a typical PR should get multiple rounds of review (if needed) within a single day.

Giving comments or feedback

How to write code review comments

Courtesy

In general, code reviewers should be courteous and respectful while being very clear and helpful to code authors of the pull request. Code reviewers must avoid making code authors upset or contentious by always commenting about the code and never commenting about authors.

• Be kind.
• Be explicit. Remember, people don’t always understand your intentions online.
• Be humble. (I’m not sure - let’s look it up.)
• Ask for clarification. (I didn’t understand. Can you clarify?)
• Avoid selective ownership of code. (mine, not mine, yours)

    `@user_id` -> This solution is better if compared to yours
  

    `@user_id` -> I think we can use this solution since it's a better approach
  

• Avoid making demands.

    `@user_id` -> Change this variable name to "NEW_VARIABLE_NAME"
  

    `@user_id` -> What do you think about naming this "NEW_VARIABLE_NAME"?
  

• Avoid subjective opinion.

    `@user_id` -> I don't like this. Mine is the correct one.
  

    `@user_id` -> We can improve this by using/reducing/updating
  

• Avoid using terms that could be seen as referring to personal traits. (dumb, stupid). Assume everyone is intelligent and well-meaning.
• Don’t use hyperbole. (always, never, endlessly, nothing)

Giving guidance

In general, the responsibility of the author is to fix the pull request, not the code reviewer. The code reviewers are not required to give a detailed solution or write code for the author of the pull request.

• When giving guidance, the code reviewer should explain the reasoning why. If there are concerns or questions from the author, the code reviewer might consider bringing the topic to Slack for discussions or event calls if there is too much confusion. After that, post a follow-up comment summarizing the PR discussion.
• Encourage code authors to simplify the code or add code comments instead of just explaining the complexity.
• Balance giving explicit direction with just pointing out problems and letting code authors decide.
• Accept that many programming decisions are opinions. Discuss tradeoffs, which you prefer, and resolve them quickly.

Handling pushback in code reviews

Who is right?

• The author is usually closer to the code than the reviewer so that the author might have a better insight into the solution. In case the author disagrees with the suggestion, the code reviewer must take a moment to consider the argument. Check if the agreement makes sense from the code health perspective. Then let the author know they are right and wrap up the issue.
• Sometimes, explaining a suggestion takes a few rounds before it sinks in. Just make sure always to stay polite and calm.

Giving approval

• In certain situations, if the reviewer is confident that the developer will appropriately address all the reviewer’s remaining comments or the remaining changes are minor and don’t have to be done by the developer, the reviewer could give LGTM or Approval even though they also leave unresolved comments on the PR.
• The reviewer should specify which of these options they intend if it is not otherwise clear.
• Here are some examples:
  • Optional: I think this may be a good idea, but it’s not strictly required.
  • FYI: I don’t expect you to do this in this PR, but you may find this interesting to think about for the future.