Skip to main content

Code Review Guidelines

At Bitwarden, we encourage everyone to participate in code reviews. A team will focus primarily on their own code reviews, but if you see something interesting, feel free to jump in and discuss.

To have efficient code reviews there are a few things to keep in mind (from Best Practices for Code Review | SmartBear):

  • Pull requests should be manageable. If the PR is too large -- significantly above a few hundred lines -- ask the contributor if it can be split it up into multiple PRs before reviewing.
    • This can be tricky in our code base since many things are tightly coupled.
  • Take your time when reviewing - expect a rate of less than 500 lines of code per hour.
  • Take breaks - don’t review for longer than 60 minutes.

Don’t feel bad for taking your time when doing code reviews! They often take longer than you think, and we should be spending as much time as needed.

tip

Bugs or defects found early in the development cycle have a much smaller cost associated with fixing them.

Reviewing​

If you feel that someone else has good knowledge of the code you are reviewing, please feel free to reach out to them or add them as a reviewer. Bitwarden developers can use the SME Yellowpages to look for knowledge area experts.

Please do not approve code you do not understand the implications of. Comments and concerns are always welcome! For example, it’s okay to leave some general comments or feedback, while also saying that you don’t have enough knowledge to approve the changes. The author can ask for another review from someone else, and there’s nothing wrong in having two reviewers on a PR.

Note

When reviewing code, remember that all software is built to conform to a set of assumptions. Features, bug fixes, and other requirement changes represent a change in those assumptions. Code, after merge, should represent the best solution that fulfills the new set of requirements, which may not necessarily be in line with the previous solution.

Review statuses​

Please use the review statuses appropriately.

Comment​

Comment is a great way to discuss things without explicitly approving or requesting changes.

Request changes​

Request changes should be used when you believe something needs to change prior to the PR getting merged, as it will prevent someone else from approving the PR before your concerns have been tackled.

We shouldn’t hesitate to use this status, however we should give clear feedback on what needs to change for the PR to get approved. Likewise a PR author should not be discouraged by a request for changes, it's simply an indication that changes should be made prior to the PR being merged. This is common.

Discarding reviews

While it’s possible to discard reviews, this should be used sparingly. Whenever possible please reach out to the reviewer first to ensure their concerns have been resolved. Below are a couple of scenarios where discarding the reviews are generally seen as accepted.

  • The reviewer is out of office for a longer duration and their original feedback has been resolved. It’s the responsibility of the new reviewer to ensure the original feedback has been addressed.
  • The PR is a hotfix that needs to be deployed quickly, and the reviewer is in a different timezone. The feedback can be addressed in a follow up PR, if it hasn't already have been resolved.

Approve​

Approving a PR means that you have confidence in that the code works, and that it does what the PR claims. This can be based on testing the change or on previous domain knowledge.

  • The PR targets the correct branch.
  • You have verified that the linked Jira issue description matches the changes made in the PR.
  • You have read and understood the full impact of the changes suggested by the PR.
  • You attest that the changes

If you are unsure about any of the above, consider using a different status or check in with the author to discuss things first. Also don’t hesitate to request a second review from someone else.

If a PR affects multiple teams, approval will be required by all teams impacted. The approver for the team that produced the PR1 is responsible for approving the change as a whole, while impacted teams are responsible only for their portion of the codebase.

Reviewing Techniques​

There are no one-size-fits-all technique for reviewing code. However there are techniques, tools, and other resources that can help you review code more efficiently.

Multiple Focus Areas​

It can be helpful to split the code review into multiple focus areas. And focus on a single view at a time.

  • Macro View - Focus on the PR as a whole.
    • Is the problem being solved?
    • Is it being solved efficiently by changing in the appropriate places using the appropriate abstractions?
    • Does the PR change the areas you expect to be changed?
      • Are any missing?
      • Are any present you didn't expect?
  • Micro View - Focus on individual files.
    • Is the code style adhered?
    • Is the code readable?
    • Are previous patterns followed?
    • Are previous patterns still the right choice?

GitHub features​

The GitHub interface has some handy tools to help you review code. For more information, see the following articles.

  • Commenting on a pull request - GitHub Docs how to comment on a PR, including:
    • commenting on multiple lines
    • suggesting code changes that the author can immediately accept and merge via the GitHub interface
      • Be careful with this! You don't have the benefit of your IDE. It's easy to break syntax or formatting.
  • About comparing branches in pull requests - GitHub Docs different ways to view the diff, including:
    • hiding whitespace (very useful if there have been lots of indentation changes)
    • displaying the old code and the new code separately (so you can read the new code without all the noise) - or combining them (so you can see exactly what’s changed)

Running Locally​

Many changes can be reviewed online on GitHub. However, sometimes it’s useful to run the code locally to improve your understanding - for example:

  • To use IDE features (like jumping to definitions or finding references)
  • To reproduce a bug you think you’ve spotted in the code
  • To run the solution to understand how it all fits together (macro view).

To run the code locally, we recommend using the GitHub CLI. This lets you checkout a PR directly without managing remote branches - for example:

// From within the repo:
gh pr checkout <GitHub PR number>

Footnotes​

  1. Or the team shepherding the PR, if it originated from the community ↩