Mastering Code Review: Standards, Guidelines, and Best Practices

Original source: https://google.github.io/eng-practices/review/reviewer/

Glossary:

CR: code review

CL: change list, referring to this change

reviewer: the person reviewing the CR

nit: short for nitpick, meaning picking at minor issues

author: the developer of this CL, called "author" in the original text

If the article is too long, you can jump to the summary at the end.

CR Standards

Code review (CR) aims to ensure that Google's codebase continuously improves in quality. All related tools and processes exist to serve this goal, requiring a series of trade‑offs and compromises.

First, developers must be able to make progress on their own tasks. If a developer never submits improvements, the codebase will never improve. Likewise, if a reviewer makes CRs difficult, developers will be reluctant to submit future changes.

On the other hand, reviewers are responsible for the quality of each change list (CL) and must ensure that overall code quality does not degrade. This can be tricky, especially when time pressure forces shortcuts that may require code to be downgraded to get it to run.

Reviewers also own the code they review and are responsible for its consistency and maintainability, as described in the section "What you can get from a CR".

Therefore, we have the following rules as the standards we expect in a CR:

Generally, when a CL exists, the reviewer should approve it, as it will improve the overall system quality even if the CL is not perfect. This is the primary principle of all CRs. Of course, there are limits. For example, if a CL adds a feature the reviewer does not want in the system, the reviewer can reject it even if the code is well designed.

A key point is that there is no "perfect" code, only better code. Reviewers should not demand polishing of every tiny part before approval. Instead, they should balance development needs with the importance of the changes, aiming for continuous improvement rather than perfection.

Reviewers should feel free to leave comments suggesting better approaches; if the comment is not critical, they can prefix it with "nit:" so the author knows it can be ignored.

Guidance

CR has an important function of teaching developers new language, framework, or general software‑design principles. Leaving comments that help developers learn is encouraged. If a comment is purely educational and not essential to meeting the standards, prefix it with "nit:" or otherwise indicate that the author does not need to address it in this CL.

Principles

Reality and data trump personal preference. In code style, the style guide is the absolute authority. Anything not covered by the style guide is a matter of personal preference. Code style should be consistent with existing code. If a project has no established style, accept the author's style.

Software design is not just about style or personal preference; it is based on fundamental principles. If an author can prove (with data or principled facts) that their approach is equally effective, the reviewer should accept the author's preference. Otherwise, design choices should follow standard software‑design principles.

If no other rule applies, the reviewer can require the author to stay consistent with the existing codebase as long as the change does not degrade overall code health.

Resolving Conflicts

In any CR conflict, the first step should always be for the developer and reviewer to reach consensus based on this document and the "CL Author’s Guide".

If consensus is especially difficult, the reviewer and author should hold a face‑to‑face meeting rather than trying to resolve the conflict solely through CR comments (but record the discussion in the CL comments for future readers).

If that does not resolve the issue, the most common solution is escalation: involve the team leader or a technical manager to make a decision. Do not let a third party stand by while the author and reviewer cannot agree.

What to Look for in a CR

Note: When considering each point, always keep the CR standards in mind.

Design

Look at the overall design of the CL. Do the different code sections interact meaningfully? Does the change belong to your codebase or an imported one? Is it well integrated with the rest of the system? Is now the right time to add this feature?

Functionality

Does the CL do what the developer intended? Does the design benefit the end users and the developers who will later use the code?

Usually, reviewers expect developers to test the CL thoroughly. Reviewers should also consider edge cases, think like a user, and ensure that reading the code alone does not reveal obvious errors.

If possible, verify the CL yourself. For UI changes, testing the visible impact is critical. If testing is inconvenient, ask the developer to demonstrate the feature.

During a CR, pay special attention to concurrent programming that could cause deadlocks or race conditions. These issues often require careful human review rather than automated testing.

Complexity

Determine whether the CL is more complex than necessary. Check if any single line, function, or class is overly complex, which can make the code hard to read and increase the risk of bugs.

One form of complexity is over‑engineering, where code is made overly generic or adds unnecessary features. Encourage developers to solve the problems they currently need rather than speculating about future needs.

Donald Knuth said: "Premature optimization is the root of all evil".

Testing

Treat unit, integration, and end‑to‑end tests as required changes for the CL. Tests should be included unless the CL is an urgent fix.

Ensure tests are correct, reasonable, and useful. Tests should not exist merely for their own sake; they must provide meaningful assertions and be appropriately separated.

Remember that test code is also code that must be maintained.

Naming

Has the developer chosen appropriate names for everything? Good names should convey purpose clearly without being overly long.

Reference "Clean Code 101 — Meaningful Names and Functions" for guidance.

Comments

Are comments clear, written in understandable English, and truly necessary?

Comments should explain *why* something was done, not *what* the code does. If the code itself cannot explain its purpose, it likely needs simplification.

Review existing comments for TODOs or suggestions that can be removed.

Note that comments differ from class, module, or function documentation, which should describe purpose, usage, and behavior.

Style

Google provides style guides for all major languages. Ensure the CL follows the appropriate guide.

If you want to improve style points not covered by the guide, prefix the comment with "Nit:" so the developer knows it is a minor, non‑mandatory suggestion. Do not block a CL solely because of personal style preferences.

Developers should not mix major style changes with other code changes in the same CL, as this makes the CL harder to understand and increases merge/rollback complexity.

Documentation

If the CL changes build, test, interaction, or release processes, update the relevant documentation (README, g3doc pages, generated reference files). If code is removed or deprecated, consider removing or updating the corresponding docs.

Every Line of Code

Review every line assigned to you. Some files (data files, generated code, large data structures) can be skimmed, but never assume that class or function code is error‑free without understanding it.

If the code is too complex and slows the review, let the developer know and wait for clarification before proceeding.

If you are not qualified to review a part (e.g., security, concurrency, accessibility, i18n), ensure a qualified reviewer is involved.

Context

Viewing the CL with sufficient context is helpful. CR tools often show only a few surrounding lines; sometimes you need to view the whole file to understand the change.

Think about the CL from a system‑wide perspective: does it improve overall code quality or add unnecessary complexity? Does it lack tests? Avoid accepting changes that degrade the system.

Pros

When you see strengths in a CL, tell the developer. Praise good practices as much as you point out problems.

Encouragement motivates developers to maintain high standards.

Summary

When performing a CR, be sure to ensure:

Code is well‑designed

Functionality is good for users

Any UI changes are reasonable and aesthetically pleasing

Parallel programming implementations are safe

Code is not overly complex

Developers do not implement features that are not currently needed

Code has appropriate unit tests

Tests are well‑designed

Developers use clear, meaningful names

Comments are clear, useful, and explain *why* rather than *what*

Documentation is updated appropriately

Code style follows the style guide

Review every line, understand context, improve code quality, and praise good work

How to Browse a CL

Now that you know what to look for, how can you efficiently review changes spread across multiple files?

Is the change reasonable? Does it have a good description?

Prioritize the most important changes. Does the overall design make sense?

Review the remaining changes in a logical order.

Step 1: Take a macro view of the change, read the CL description and understand what it does.

If the change seems unreasonable, explain why it should not happen and suggest an alternative.

When rejecting, remain polite and provide a constructive suggestion.

If multiple CLs contain unwanted changes, reconsider the development process or communicate the workflow to external contributors.

Provide alternatives so the author knows what to do next.

Step 2: Check the main parts of the CL.

Identify the core files that contain the bulk of the logic and review them first. If the CL is too large to identify the core, ask the developer for guidance or request that the CL be split.

If major design issues are found early, comment immediately, as continuing to review other parts would be wasteful.

Why Review Speed Matters

Google optimizes team‑wide product development speed, not individual speed. Slow reviews delay features and bug fixes, cause developer frustration, and can degrade code quality.

How Fast Should a CR Be?

If you are not in a focused work period, aim to review a CL as soon as possible, ideally within one workday.

Speed vs. Interruptions

Sometimes personal focus should take precedence. Avoid interrupting yourself for a CR if you need to concentrate on coding.

Research shows that interruptions significantly increase the time to resume productive work.

Instead, find a suitable break point (after finishing a task, during lunch, etc.) to perform the CR.

Quick Responses

When we talk about CR speed, we refer to response time, not how long the CL takes to complete.

Fast responses from reviewers reduce developer frustration even if the overall process takes longer.

If you are very busy, at least acknowledge when you will start the review or suggest another reviewer.

Cross‑Timezone Reviews

When dealing with different time zones, try to reply while the author is still in office, or before they return the next day.

LGTM Comments

Reviewers can give an LGTM or Approval even if some comments remain unresolved, provided they are confident the author will address them, the remaining comments are trivial, and the reviewer clearly indicates the situation.

Large Changes

If a CL is too large to review effectively, ask the developer to split it into smaller CLs. If splitting is impossible and you lack time, at least comment on the overall design and send it back for improvement.

CR Improves Over Time

Following these guidelines makes the CR process faster as developers learn to write higher‑quality code and reviewers become more efficient.

Emergency Situations

In emergencies, standards may be relaxed to get the change through quickly. Understand what qualifies as an emergency.

How to Write Review Comments

Dealing with Delayed Comments

Sometimes developers postpone handling review comments because they disagree or feel the reviewer is too strict.

Who Is Right?

When a developer disagrees, consider whether they might be correct. Developers often have deeper knowledge of the code. If their argument is valid, acknowledge it and move on.

If they are wrong, explain why your suggestion improves code quality, staying polite and clear.

Avoiding Developer Frustration

Strict reviewers can unintentionally frustrate developers, but frustration is usually short‑lived and often resolved once developers see the value of higher quality code.

Postponing Clean‑up

Developers may want to defer clean‑up to a later CL. Encourage them to clean up after the CL is merged, as postponing clean‑up is a common cause of code‑base decay.

If a CL introduces new complexity, it should be addressed before submission unless it is an emergency.

Common Complaints About Review Strictness

When reviewers become stricter, developers may initially complain, but as review speed improves, complaints fade and developers appreciate the higher quality code.

Resolving Remaining Conflicts

If conflicts persist despite following all guidelines, refer back to the CR standards for additional principles.

Translator's Summary

CR Standards

Reviewers are responsible for CL quality and act as owners of the reviewed code.

Reviewers should aim for continuous improvement, not perfection.

CR provides guidance and teaching opportunities.

Code style should be consistent with existing code; if no style exists, accept the author's style.

When consensus is hard, hold face‑to‑face meetings or involve team leads.

What to Look for in a CR

Overall design of the CL

Functional verification and UI quality

Complexity and over‑engineering

Appropriate unit tests

Well‑designed tests

Clear, meaningful naming

Useful comments that explain why, not what

Adherence to style guides; separate style changes into their own CLs

Documentation updates when build, test, interaction, or release changes occur

Thorough line‑by‑line review, seeking explanations for complex code

Security, concurrency, accessibility, and i18n issues require qualified reviewers

Consider the CL from a system‑wide perspective

Praise strengths as well as point out issues

How to Browse a CL

Take a macro view: read the CL description and purpose

Identify and review the CL's main parts first

Review remaining changes in a logical order

Review Speed

Slow reviews hurt team velocity, cause developer protest, and degrade code quality

Avoid interrupting focused work for CR

Fast personal responses matter more than overall process speed

Reply promptly across time zones

LGTM can be given even with minor unresolved comments

Large CLs should be split into smaller ones

Increase CR speed without compromising standards

How to Write Review Comments

When developers disagree, consider who is right and explain politely

Strictness can cause frustration, but good communication mitigates it

Address new issues before submission unless it's an emergency

Use TODO comments and link to recorded defects when needed

Dealing with Strict Review Complaints

Improving review speed reduces complaints; over time developers see the value of strict reviews in producing excellent code.