How we do Code Review

Like many software engineering teams, code review is a required and important part of our development workflow. Engineers are required to submit a Pull Request on GitHub and get approval before merging into our master branch. However, several engineers on the team recently noted that they felt unqualified or uncertain to approve code through code review. Consequently, they were also uncertain whether their own code had been fully reviewed. We broke down this problem and identified several underlying issues:

  1. new team members weren’t comfortable reviewing code, which reduces the number of reviewers we have
  2. with the hierarchy and structure of our React code, reviewers had a hard time understanding the context of a change just from the diff
  3. some pull requests were difficult to review because they were either too big or scoped too widely

To address these issues, I gathered information from several sources to assemble a new documentation page on code review. First, I talked to software engineer peers to understand how other companies handle code review. Second, I searched online for guides and recommendations on how to do code review. Third, I looked at metrics about our pull requests to understand what our current processes are. The result is the documentation page approximately reproduced below.

Code Review

As part of our development workflow, code is submitted in a pull request (PR) and requires approval before being merged into master. Although code review is ostensibly for catching bugs and verifying code correctness, the most valuable part of code review is sharing knowledge within the engineering team.

Code review is a process of reading and discussing each others’ code. It establishes coding conventions and practices for long-term consistency. It establishes shared code ownership so different engineers can maintain the same components. Individual engineers can learn from each other and become better engineers.

Submission Tips

Good code review starts with a good pull request. Think about submitting code from the perspective of your prospective reviewer. By making their job easier, we are more likely to catch mistakes and learn from each other. Here are a few concrete suggestions:

  1. Submit small diffs. Break up refactoring and multiple tasks into separate PRs against a base branch to make the purpose of each PR as clear as possible. SmartBear researched this topic and recommends that a PR should generally be less than 200 lines of code (LoC) and never more than 400
  2. Review your own code first. Before you hit “Create Pull Request”, look over the diff in GitHub as though you were a reviewer. This scan can catch leftover debugging code, unfinished segments, or needed comments.
  3. Write a clear summary of your task in the PR description. Your reviewer likely has not read the ticket and certainly has less context than you do. Explain what you did, why you did it, and (if necessary) how you did it. If it is a UI change, provide before and after screenshots
  4. Review your own code first (again). After you post your PR, add your own comments. Explain why you did something, point out tricky code worth extra attention, and clarify confusing diffs.

Review Tips

  • Review code even if you don’t feel qualified to approve it. It’s a great way to learn.
  • Always ask questions. At minimum, you will learn something. Oftentimes, questions will lead to more documentation. Sometimes, we even find bugs.
  • Take your time. SmartBear recommends a 200-500 LoC/hour inspection rate. That sounds slower than our team typically goes, but maybe that means there’s lots of room to slow down
  • Be positive! Code review is criticism, but it is collaborative, and everyone can learn. Frame all of your comments generously and point out the good bits, too.

Here’s a general list of concerns to keep in mind during code review:

  1. Code quality – readability, documentation, robustness, conventions
  2. Security – access checks, data validation, data integrity
  3. Performance – optimization, scalability
  4. Functionality – bug checking
  5. Test coverage – unit tests, integration tests

The hardest part of code review is catching omissions where code isn’t in the diff. Most guides also recommend developing (and maintaining) personal checklists that you assemble over time about issues that got past code review. Since they’re personal, we won’t prescribe a checklist, but you’re welcome to create one for yourself.

Be prepared!

Your first code reviews will be brutal. Multiple engineers will nitpick your coding style and conventions. Sometimes they will request complete rewrites. These reviews are expected as part of learning our conventions and practices, and it just takes an adjustment period. Don’t be discouraged!

GitHub Tips

  • When reading the diff, add ?w=1 to the URL to ignore the whitespace changes. This is very helpful to see past indentation changes on big blocks of code
  • Use the “Reviewers” field and the “Labels” tags as much as you need
  • Add and remove the “work in progress” appropriately

References