Code reviews

A culture of high-quality code reviews helps engineers of all experience levels grow and promotes a shared understanding of the codebase. A poor code review culture inhibits innovation, slows down development, and builds resentment.

Why Review Code?

A well-executed code review is extremely valuable. There are obvious, superficial benefits—reviews can catch bugs and keep code clean—but a code review’s value goes beyond having humans stand in for automated tests and linters.

Good reviews act as a teaching tool, spread awareness, document implementation decisions, and provide change records for security and compliance.

Reviews are not an opportunity to prove how smart you are, nor are they a rubber-stamping bureaucratic hurdle.

Getting Your Code Reviewed

Code changes are prepared, submitted, reviewed, and finally approved and merged.

Prepare Your Review: Follow the VCS guidance that we give in Chapter 3: keep individual code changes small, separate feature and refactoring work into different reviews, and write descriptive commit messages. Include comments and tests. Don’t get attached to the code you submit for review; expect it to change, sometimes significantly, as it goes through the process.

Example:

Reviewers: agupta, csmith, jshu, ui-ux
Title: [UI-1343] Fix missing link in menu header
Description:
# Summary
The main menu header is missing a link for the About Us menu option.
Clicking the menu button does nothing right now.
Fixed by adding a proper
href.
Added a Selenium test to verify the change.
# Checklist
This PR:
- [x] Adds new tests
- [ ] Modifies public-facing APIs
- [ ] Includes a design document

De-risk with Draft Reviews: a draft review: an informal review request intended to get quick and cheap feedback from team- mates, which significantly reduces the risk that you go too far down the wrong path. Add “DRAFT” or “WIP” to the title. Once your draft feels like it’s on the right track, you can transition it out of the “draft” state by finishing the implementation, tests, and documentation, and adding polish.

Don’t Submit Reviews to Trigger Tests: It can be hard, as a new developer, to figure out how to run all relevant tests. Some developers bypass this problem by submitting code reviews to trigger the continuous integration (CI) system. This is a poor practice.

Walk Through Large Code Changes: Walk-throughs are in-person meetings where a developer shares their screen and walks teammates through the changes that are being made. Walk-throughs are a great way to trigger ideas and get your team comfortable with changes. Circulate relevant design documents and code in advance. Don’t try to get your teammates to actually review the code in the walk-through.

Don’t Get Attached: Getting critical comments on your code can be tough. Keep some emotional distance—the review is of the code, not of you, and it’s not even really your code; the whole team will own the code in the future.

Practice Empathy, but Don’t Tolerate Rudeness

Be Proactive: Don’t be shy about asking others to review your code. Reviewers are often bombarded with code review and ticket notifications, so reviews can get lost on high-velocity projects. If you don’t get any feedback, check in with the team (without being pushy).

Reviewing Code

You have to contribute to the code review process by reviewing code too.

Triage Review Requests: Priority, urgency and etc.

Block Off Time for Reviews: Don’t drop everything you’re doing every time a review request arrives. Left unchecked, review interruptions can torpedo your productivity. If you get a review that’s going to take more than an hour or two to get through, create an issue to track the review itself.

Understand the Change: Understanding the motivation for a change will explain implementation decisions, and you might discover the change isn’t even needed.

Give Comprehensive Feedback: Give feedback on a change’s correctness, implementation, maintainability, legibility, and security.

Acknowledge the Good Stuff.

Distinguish Between Issues, Suggestions, and Nitpicks: Major issues need more attention than neutral suggestions and superficial nitpicks. Don’t shy away from stylistic feedback but make it clear that you’re nitpicking. A “nit” prefix prepended to the comment is customary:

Don’t Rubber-Stamp Reviews: Resist the temptation to rubber-stamp a review with a hasty approval. If you can’t prioritize a review adequately, don’t review the change at all.

Don’t Limit Yourself to Web-Based Review Tools:

Don’t Forget to Review Tests: Tests should be reviewed just like the rest of the code.

Drive to a Conclusion: Insist on quality, but do not become an impassible barrier. (https://google.github.io/eng-practices/) Respect the scope of the change that’s being made.

Example:

Review summary:
Change looks good. Few minor nits, but my main request is to fix the porthandling.
The code there looks brittle. See my comment for details.

Note

Sources: