Code Reviews

Hero image for Code Reviews

Code is never merged directly to the development and master branch. Each feature, bug or chore branch requires a code review.

Guidelines for authors

Opening a pull request

  • Submit a pull request for each branch that needs to be reviewed
  • The name of the pull request must contain both the user story ID and title:
[<User Story ID>] As a user I can view the list of job positions
  • The description of the pull request must abide by this template:

    Pull Request Template

    • On the first line, include the link(s) to the user story(ies) to make it easier for reviewers to get the complete requirements
      # Pivotal Tracker
      https://www.pivotaltracker.com/story/show/133252875
        
      # JIRA
      https://nimblehq.atlassian.net/browse/BRAIV-18
    
    • What Happened: provide a description of the changes made. If there are some pending TODOs, include them there as well.
    • Insight: any guidance for reviewers to better understand the changes.
    • Proof of Work: any visuals that can give assurance that the changes are safe to merge.

A well-written pull request can go a long way to make code reviews speedier and overall more efficient 💪

For efficiency purposes, prefer to set up the default pull request description at the project level to avoid recreating this structure each time a new PR is opened 😰:

  • Github: Use a Markdown-based template in the repository itself similarly to the GIT template:
  • Bitbucket: Set a Markdown-based template in the repository settings: https://bitbucket.org/nimblehq/<repo-name>/admin/pullrequests/default-pull-request-description/ (replace <repo-name> with the actual repository name)
  • Do NOT wait to complete all requirements before opening a pull request. In theses cases, prepend the suffix WIP - to the pull request title.
WIP - [<User Story ID>] As a user I view the list of job positions

Work-in-progress (WIP) pull requests are encouraged to both get early feedback on an implementation and share it with the team. For instance, other team members could cherry-pick code from the branch even though it is not complete. For efficiency purposes, the Team Lead can detect issues early in the implementation choices hence rerouting efforts earlier in the sprint.

Github specific

Pull Request Template

  • Assign the appropriate labels to the PR. Labels are used to categorize the type of PR (feature, chore…) and priority (high, normal or low).
  • Assign a milestone to the PR. Usually, a milestone corresponds to a release version.

Adding reviewers

  • Between one to two reviewers must be added to each pull request: either two team members in the same squad or, for a smaller project, one team member in the project and one “outsider” (not working in the project). The “outsider” must be defined as a reviewer member of the squad in the team organization diagram.
  • The reviewers must include the Team Lead and one other developer
  • Reviewers approve the pull request when it passes the team guidelines and solves the issue at hand in a satisfying way.
  • Rebase your local branch before submitting your pull request to make it ready for merge.

Processing feedback

  • Seek to understand the reviewer’s perspective. Assume the best intention from the reviewer’s comments.
  • Be grateful for the reviewer’s suggestions e.g. Good catch. I will make that change.
  • Respond to every comment either offline (in person) or online.
  • 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
  • Extract some changes and refactorings into future tickets/stories if the requested changes are large and/or out of the initial scope of the PR.
  • Merging a pull request can only happen if between one to two reviewers approved it and it passes the tests suite.

Guidelines for reviewers

Processing a pull request

  • Each PR should be reviewed within one day. But it’s ok for PR authors to ask for a speedier code review on Slack
  • Each PR should be merged within 5 days i.e. during the current sprint or next sprint at most. Stale PRs are usually hard to merge past this time range and require difficult rebase.
  • Approve the PR by using the review tool of version control service (Github, Bitbucket or Gitlab).
  • Merge once you feel confident in the code and its impact on the project.
  • Merging requires a minimum of two review approvals. On large squads with over two developers, the approvals must include at least the Team Lead and/or one senior team teammates to avoid merges with approvals from only junior teammates.

Giving feedback

  • Comment directly on the line of code in the web interface of the version control service (Github, Bitbucket or Gitlab)
  • Use an inquisitive tone, do not make an order:
# Bad
`@user_id` -> Change this variable name to "NEW_VARIABLE_NAME"

# Good
`@user_id` -> What do you think about naming this "NEW_VARIABLE_NAME"?
  • Ask for clarification e.g. I didn't understand. Can you please explain?
  • Avoid selective ownership of code. (mine, not mine, yours)
  • Accept that many programming decisions are opinions. Engage a discussion and reach a resolution quickly.
  • Seek to understand the author’s perspective.
  • Identify ways to simplify the code while still solving the problem.