Similar to our design reviews, we strive for our code reviews to be:
- Collaborative, not combative.
- The goal of the review is to increase the quality of the software and the expertise of the team. Ideas should be discussed in a neutral way, irrespective of their authors. It is not the goal of the review to defend or attack the change being reviewed, but to take advantage of additional feedback.
- On correctness, not agreement.
- Reviewers should limit their comments to problems, or things that can cause problems. It is not the goal of the review to force the author to solve a problem the way the reviewer would.
- Focused, not theoretical.
- Review comments should be focused on specific issues. Generalizing and expanding beyond the specific change being reviewed, to opinions, aesthetics, style, hypothetical scenarios, or philosophical conversations are outside the scope of a code review.
- Encompassing.
- Anything that needs to be checked in should get reviewed, with the exception of creating a new branch. This eliminates the question of whether something warrants a code review. Trivial changes should be trivial to review.
- Proactive.
- Significant design decisions should be shared with the reviewer early in the process, to avoid major implementation changes based on late feedback. Additional people can be consulted during a design review, at the discretion of either the author or the reviewer.
- Precommit.
- Changes should be reviewed before being checked in and not after. Methods such as branching or shelving can achieve that.
- Descriptive.
- A review request should provide a high-level description of the change.
- Clearly assigned.
- A review can be requested of more than one person, at the discretion of either the author or the reviewer. In those cases it should be clear whether more than one full review is requested, or whether additional people are merely consulted.
- Unblocked.
- On rare occasions the reviewer and the author may reach a disagreement that they cannot resolve or compromise on. A second reviewer provides the final decision in that case, after examining the arguments of both sides.
- Timely.
- A review request should allow enough time for any potential feedback to be discussed and incorporated.
- Lightweight.
- A review request should ideally include less than 300 lines of changes and definitely less than 500 lines.
- Timeboxed.
- A review should ideally take less than 60 minutes and definitely less than 90 minutes.
- Responsive.
- A review should ideally be completed within a day and definitely within two days.
- Not requiring comments.
- It is acceptable for the output of a review to be just an approval. Alternatively, a list of suggestions (verbal or written) can be presented to the author.
- Excluding, including, or punting.
- If there are suggestions, i) the reviewer and the author can decide that they don't need to be addressed, ii) the author can incorporate them, or iii) the author can create artifacts to capture them as future work. It is both the author's and reviewer's responsibility to decide on i). It is the author's responsibility to perform ii) or iii). It is the reviewer's responsibility to track ii) and iii) before approving a review request.
- Limited.
- If any follow-up (verbal or written) is needed, to discuss the review output and decide which suggestions fall under i), ii), and iii), it should ideally take less than 15 minutes and definitely less than 30 minutes.
- Fun.
- Don't forget to have fun.
cd /home