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:
- new team members weren’t comfortable reviewing code, which reduces the number of reviewers we have
- with the hierarchy and structure of our React code, reviewers had a hard time understanding the context of a change just from the diff
- 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.
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.
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:
- 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
- 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.
- 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
- 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 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:
- Code quality – readability, documentation, robustness, conventions
- Security – access checks, data validation, data integrity
- Performance – optimization, scalability
- Functionality – bug checking
- 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.
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!
- 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
- Atlassian blog: Code Review in Agile Teams – short article on code review principles
- Asana blog: 7 ways to uplevel your code review skills – more code review principles
- SmartBear: Best Practices for Code Review – concrete statistics and recommendations on code review
- Best Kept Secrets of Peer Code Review – this collection of essays is long but well-researched and justifies much of the blog post above
- Beanstalk Guides: Code Review workflow
- Push-pull – a hackathon project to generate pull request statistics graphs from GitHub. It’s not polished, but it could be insightful