Code Review¶
Introduction¶
What is code review for?
For many, the answer is "catching bugs". This guide takes a different view – review is for:
- training and knowledge sharing;
- team cohesion;
- architectural and pattern alignment; and
- de-siloing;
Extended standards and guidelines
This page is a short-form reference guide.
For the full standards and guidelines, see the Flanksource Code Review Guidelines.
Standards¶
When should a pull request be approved?¶
In general, reviewers should favour approving a pull request where:
- it is in a state where it definitely improves the overall code health of the system being worked on;
- where reasonable, all opportunities for training and consensus development have been taken and results captured;
- it fulfils the architectural ambitions and meets any standards, definitions of done, or the requirements of other checklists and processes; review has been appropriately thorough and the reviewer understands the changes;
- it has no easy-to-fix, clear and obvious errors without guaranteed remediation path;
- all comments have been acknowledged and appropriately addressed;
- all reasonable change requests have been addressed;
- all work arising from the review is captured; and
- no automated checks are failing.
These conditions do not apply in a production emergency, but it is expected that code which is pushed to resolve an emergency will be revisited.
How quickly should a pull request be attended to?¶
Team members should respond to requests for review shortly after they come in and as close to immediately as is reasonable – unless in the middle of a focused task, such as writing code. Even then, reviews should be undertaken as soon as a natural breakpoint arises, such as when a coding task or test is completed or a developer pauses to make coffee.
First response and partial reviews¶
Reviewers should attempt to give fast first-pass reviews to provide rapid feedback. A first pass may include:
- a read of whether the solution is sensible or not – if it's clearly going wrong, the reviewer should catch this here and not request a larger set of changes;
- notes on obvious mistakes and code hygiene; and
- notes on missing features, tests and/or mismatches with requirements.
Maximum response time¶
The maximum response time for review should be one business day, barring unusual circumstances.
What should reviewers be looking for?¶
- Functionality
- Standards and architecture
- Cruft and technical debt
- Complexity, clarity & performance
- Product fit
- Documentation & comments
- Testing
- Deployability
- Security, privacy and compliance
- Altruism
- Observability
Rules¶
Approval rules¶
There should be at least one (non-author) reviewer of code and this rule should be set on the repo as a merge guard. If a team is large enough, say five or more developers, then this number should be two.
In most cases, one approver is an effective number; however, more may be useful at the start of the project where technical leads and architects might want an overview of all code.
Approval should void on changes¶
When code is approved and then changes, approval should be revoked. Approvers are responsible for the code they approve, and a means for changes to be made that they're in principle unaware of should be avoided.
Assignees versus reviewers¶
It can be useful to distinguish between assignees and reviewers. An assignee is ultimately responsible for merging. Reviewers should be knowledgeable people whose input into the code will improve it or those who need to be aware of changes (or who need to be introduced to the system in the first place).
Number of reviewers¶
Where reviews might have only one approver, it is good practice for there to be many eyes on code. Consequential changes especially should have multiple reviewers.
Further reading¶
When should a pull request be approved?¶
The Bunny Theory of Code, Nicholas C. Zakas
The Standard of Code Review, Google Engineering Practices
How quickly should a pull request be attended to?¶
Speed of Code Review, Google Engineering Practices
What should reviewers be looking for?¶
The Standard of Code Review, Google Engineering Practices
What to look for in a code review, Google Engineering Practices
Code Only Says What it Does, Marc Brooker
Maslow's pyramid of code review, Charles-Axel Dein
Code Review Best Practices - Lessons from the Trenches, Drazen Zaric
Giving better code reviews, Joel Kemp
How to prepare code¶
A Guide for Code Authors, Chromium Project
Small CLs, Google Engineering Practices
How to comment on code¶
How to write code review comments, Google Engineering Practices
A Guide for Code Reviewers, Chromium Project
Feedback Ladders: How We Encode Code Reviews at Netlify, L. Cohn-Wein, K. Lavavej, S. Wang
Code Review Guidelines for Humans, Phillip Hauer
How to respond to comments¶
Handling pushback in code reviews, Google Engineering Practices