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

  • Submit a pull request for the 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 contain the link to the user story to make it easier for reviewers to get the complete requirements e.g. https://www.pivotaltracker.com/story/show/133252875.
  • At least two reviewers should be added to each pull request: either two team members in the project or, for a smaller project, one team member in the project and one “outsider” (not working in the project).
  • Reviewers approves 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.
  • Do not wait to complete all requirements before opening a pull request. In this cases, append the suffix [WIP] to the pull request title.
[<User Story ID>] As a user I view the list of job positions [WIP]

Work in progress 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.

  • 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 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 both reviewers approved it and it passes the tests suite.

Github specific * 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.

Guidelines for reviewers

  • Accept that many programming decisions are opinions. Engage a discussion and reach a resolution quickly.
  • 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)
  • Seek to understand the author’s perspective.
  • Identify ways to simplify the code while still solving the problem.
  • PRs should be reviewed within one day. But it’s ok for PR authors to ask for a speedier code review on Slack
  • PRs 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.