Pull Request Feedback

Checklist

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 Pull Requests

High-level Review

• Look at the pull request’s title and description and what the PR does in general. Does it follow the correct structure guidelines?

• Look at the story/issue provided in the first line of the PR’s description to get all the Acceptance Criteria (AC), to have the 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 can focus on the following checks:

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

Providing Feedback

Courteous Language

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

• 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 the code reviewers prefer, and resolve them quickly.

Processing Feedback

Responding to Feedback

• Avoid responding negatively to code review comments. Seek to understand the reviewer’s perspective. Authors should always ask themselves, “What is the constructive thing the reviewer is trying to communicate to me?”. Assume the best intention from the reviewer’s comments.

• Be grateful for the reviewer’s suggestions (e.g., Good idea. I will make that change.).

• Acknowledge when mistakes were spotted (e.g., Good catch, I missed that. Fixing it now.).

• Prefer to respond to every comment by replying with a text message. Avoid only adding an emoji reaction to the review comment unless 1) a similar comment was already addressed with a proper response or 2) the review comment is minor, e.g., suggesting a text replacement, pointing out a typo, etc.

• Push commits based on rounds of feedback as isolated commits to the branch. As a good practice, reply to the comment with a link to fix (e.g.,Fixed in a4994ec).

• Respond timely to new feedback. Use tools (e.g., Email, Slack bot, etc.) to get notifications from pull requests.

• The code reviewer would be the one closing the comments most of the time, however, the author can mark a review comment as resolved when:

  • The solution is clearly suggested/written in the initial comment — i.e., using change suggestion.
  • When the comment mentions specific typos.
  • When the change request is evident to the author and it was just a miss-out, e.g., basic conventions, code formatting, forgetting to run linter, removing a file from code versioning, etc.

Unlocking Stalemate Situations

When the code reviewer and the author cannot agree on a solution, the following guidelines should be taken to efficiently unlock the stalemate situation:

• Ask the code reviewer to reconsider the suggestion. The author is usually closer to the code than the reviewer so the author might have a better insight into the solution. Check if the agreement makes sense from the code health perspective.

• Explain a suggestion from a different perspective. Sometimes, a suggestion takes a few rounds before it sinks in. Just make sure always to stay polite and calm. Do not hesitate to use video calls to discuss the issue if needed.

• Escalate the issue to the Team Lead. The Team Lead is responsible for making all technical arbitration decisions.

Approvals

Purpose

The purpose of the code review process is for the team to have confidence in the code and its impact on the project. Therefore, a code review is NOT a rubber stamp.

The code reviewer must only approve a pull request if:

• They are confident that the code is correct and will not cause problems, such as bugs and maintenance overhead.
• They are confident that the code is consistent with the rest of the codebase.
• They understand the code and can maintain it in the future.
• They have exhausted all the valid and possible improvements to the code that can be performed within the project’s constraints.

Pre-approving

A pull request can be pre-approved to speed up code merges in two instances:

• The remaining changes are minor and do not affect the code’s correctness.
• The proposed suggestions do not have to be done in the current pull request.

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 future implementation.