Code review

Definition

A code review is a process where someone other than the author(s) of a piece of code examines that code.

The primary purpose of code review is to make sure that the overall code health of Google’s codebase is improving over time. All of the tools and processes of code review are designed to this end.

The gold standard in code reviews:

In general, reviewers should favor approving a PR once it is in a state where it definitely improves the overall code health of the system being worked on, even if the PR isn’t perfect.

There are limitations to this, of course. For example, if a CL adds a feature that the reviewer doesn’t want in their system, then the reviewer can certainly deny approval even if the code is well-designed.

A key point here is that there is no such thing as “perfect” code—there is only better code

Code review can have an important function of teaching developers something new about a language, a framework, or general software design principles. It’s always fine to leave comments that help a developer learn something new. Sharing knowledge is part of improving the code health of a system over time.

Principles

  • Technical facts and data overrule opinions and personal preferences.

  • On matters of style, the style guide is the absolute authority. Any purely style point (whitespace, etc.) that is not in the style guide is a matter of personal preference. The style should be consistent with what is there. If there is no previous style, accept the author’s.

  • Aspects of software design are almost never a pure style issue or just a personal preference. They are based on underlying principles and should be weighed on those principles, not simply by personal opinion. Sometimes there are a few valid options. If the author can demonstrate (either through data or based on solid engineering principles) that several approaches are equally valid, then the reviewer should accept the preference of the author. Otherwise the choice is dictated by standard principles of software design.

  • If no other rule applies, then the reviewer may ask the author to be consistent with what is in the current codebase, as long as that doesn’t worsen the overall code health of the system.

What to look for in a code review

Code reviews should look at:

  • Design: Is the code well-designed and appropriate for your system?

  • Functionality: Does the code behave as the author likely intended? Is the way the code behaves good for its users?

  • Complexity: Could the code be made simpler? Would another developer be able to easily understand and use this code when they come across it in the future?

  • Tests: Does the code have correct and well-designed automated tests?

  • Naming: Did the developer choose clear names for variables, classes, methods, etc.?

  • Comments: Are the comments clear and useful?

  • Style: Does the code follow our style guides?

  • Documentation: Did the developer also update relevant documentation?

In doing a code review, you should make sure that:

  • The code is well-designed.

  • The functionality is good for the users of the code.

  • Any UI changes are sensible and look good.

  • Any parallel programming is done safely.

  • The code isn’t more complex than it needs to be.

  • The developer isn’t implementing things they might need in the future but don’t know they need now.

  • Code has appropriate unit tests.

  • Tests are well-designed.

  • The developer used clear names for everything.

  • Comments are clear and useful, and mostly explain why instead of what.

  • Code is appropriately documented (generally in g3doc).

  • The code conforms to our style guides.

Make sure to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on the good things that they do.

The most efficient way to manage a review that’s spread across multiple files:

  1. Does the change make sense? Does it have a good description?

  2. Look at the most important part of the change first. Is it well-designed overall?

  3. Look at the rest of the CL in an appropriate sequence.

Code review speed

When code reviews are slow, several things happen:

  • The velocity of the team as a whole is decreased. Yes, the individual, who doesn’t respond quickly to the review, gets other work done. However, new features and bug fixes for the rest of the team are delayed by days, weeks, or months as each PR waits for review and re-review.

  • Developers start to protest the code review process. If a reviewer only responds every few days, but requests major changes to the PR each time, that can be frustrating and difficult for developers. Often, this is expressed as complaints about how “strict” the reviewer is being. If the reviewer requests the same substantial changes (changes which really do improve code health) but responds quickly every time the developer makes an update, the complaints tend to disappear. Most complaints about the code review process are actually resolved by making the process faster.

  • Code health can be impacted. When reviews are slow, there is increased pressure to allow developers to submit PRs that are not as good as they could be. Slow reviews also discourage code cleanups, refactorings, and further improvements to existing PRs.

If you are not in the middle of a focused task, you should do a code review shortly after it comes in.

One business day is the maximum time it should take to respond to a code review request (i.e. first thing the next morning). Following these guidelines means that a typical PR should get multiple rounds of review (if needed) within a single day.

There is one time where the consideration of personal velocity trumps team velocity. If you are in the middle of a focused task, such as writing code, don’t interrupt yourself to do a code review.

When dealing with time zone differences, try to get back to the author when they are still in the office. If they have already gone home, then try to make sure your review is done before they get back to the office the next day.

How to write code review comments

Principles:

  • Be kind - One way to do this is to be sure that you are always making comments about the code and never making comments about the developer.

  • Explain your reasoning - help the developer understand why you are making your comment

  • Balance giving explicit directions with just pointing out problems and letting the developer decide - you should strike an appropriate balance between pointing out problems and providing direct guidance. Pointing out problems and letting the developer make a decision often helps the developer learn, and makes it easier to do code reviews.

  • Encourage developers to simplify code or add code comments instead of just explaining the complexity to you - If you ask a developer to explain a piece of code that you don’t understand, that should usually result in them rewriting the code more clearly. Occasionally, adding a comment in the code is also an appropriate response, as long as it’s not just explaining overly complex code.

Resolving Conflicts

When a developer disagrees with your suggestion, first take a moment to consider if they are correct. Often, they are closer to the code than you are, and so they might really have a better insight about certain aspects of it. Does their argument make sense? Does it make sense from a code health perspective? If so, let them know that they are right and let the issue drop.

However, developers are not always right. In this case the reviewer should further explain why they believe that their suggestion is correct. A good explanation demonstrates both an understanding of the developer’s reply, and additional information about why the change is being requested.

In any conflict on a code review, the first step should always be for the developer and reviewer to try to come to a consensus.

When coming to consensus becomes especially difficult, it can help to have a face-to-face meeting or a video conference between the reviewer and the author, instead of just trying to resolve the conflict through code review comments. (If you do this, though, make sure to record the results of the discussion as a comment on the PR, for future readers.)

If that doesn’t resolve the situation, the most common way to resolve it would be to escalate. Often the escalation path is to a broader team discussion, having a Technical Lead weigh in, asking for a decision from a maintainer of the code, or asking an Eng Manager to help out. Don’t let a PR sit around because the author and the reviewer can’t come to an agreement.

Prefer small PRs

Small PR benefits:

  • Reviewed more quickly. It’s easier for a reviewer to find five minutes several times to review small PRs than to set aside a 30 minute block to review one large PR.

  • Reviewed more thoroughly. With large changes, reviewers and authors tend to get frustrated by large volumes of detailed commentary shifting back and forth—sometimes to the point where important points get missed or dropped.

  • Less likely to introduce bugs. Since you’re making fewer changes, it’s easier for you and your reviewer to reason effectively about the impact of the PR and see if a bug has been introduced.

  • Less wasted work if they are rejected. If you write a huge PR and then your reviewer says that the overall direction is wrong, you’ve wasted a lot of work.

  • Easier to merge. Working on a large PR takes a long time, so you will have lots of conflicts when you merge, and you will have to merge frequently.

  • Easier to design well. It’s a lot easier to polish the design and code health of a small change than it is to refine all the details of a large change.

  • Less blocking on reviews. Sending self-contained portions of your overall change allows you to continue coding while you wait for your current PR in review.

  • Simpler to roll back. A large PR will more likely touch files that get updated between the initial PR submission and a rollback PR, complicating the rollback (the intermediate PRs will probably need to be rolled back too).

There are a few situations in which large changes aren’t as bad:

  • You can usually count deletion of an entire file as being just one line of change, because it doesn’t take the reviewer very long to review.

  • Sometimes a large PR has been generated by an automatic refactoring tool that you trust completely, and the reviewer’s job is just to verify and say that they really do want the change. These PRs can be larger, although some of the caveats from above (such as merging and testing) still apply.

It’s usually best to do refactorings in a separate PR from feature changes or bug fixes. For example, moving and renaming a class should be in a different PR from fixing a bug in that class. It is much easier for reviewers to understand the changes introduced by each PR when they are separate.

If you have several PRs that depend on each other, you need to find a way to make sure the whole system keeps working after each PR is submitted. Otherwise you might break the build for all your fellow developers for a few minutes between your PR submissions (or even longer if something goes wrong unexpectedly with your later PR submissions).

Unhelpful behavior

  • passing off opinion as fact - Don’t make claims unless you can cite documentation, formalized guidelines, and coding examples to back those claims up. People need to know why they are being asked to make a change, and another developer’s personal preference isn’t a good enough argument.

  • overwhelming with an avalanche of comments - Consolidating comments allows you to convey the same message without overwhelming the review seeker. An avalanche of comments for one problem, duplicated, comes off as nitpicking.

  • asking engineers to solve problems they didn’t cause “while they are at it” - Don’t ask devs to fix problems “while they are at it.” If the code solves the issue/ticket the dev was working on and doesn’t introduce any new issues to the codebase, give the pull request a “thumbs up” and then create a ticket to clean up the bad code

  • asking judgmental questions - Asking such questions implies that a perceived simple solution should have been obvious. It also forces developers to have to defend themselves. Instead, provide a recommendation (with documentation and citations) and leave out harsh words.

  • being sarcastic - There is no appropriate time to be sarcastic when offering someone feedback. Sarcastic comments tend not to provide context or actionable feedback. Instead, describe the issue with details and provide recommendations, but leave the caustic jokes out.

  • using emojis instead of statements to point out issues - Avoid using the thumbs-down or puke emoji to point out issues in code. This is as unhelpful as sarcasm for similar reasons. Emojis are cryptic and easy to misconstrue. Emojis waste peoples’ time as they try to figure out what you mean.

  • not replying to all comments - If a comment is out of scope or you won’t be taking action on the feedback, just provide a brief note explaining why.

  • ignoring toxic behaviors from high performers - Toxic behaviors should not be ignored or deemphasized because a developer is a high performer and extremely productive. Though this developer might be doing a fantastic job, it is important to keep in mind that this developer’s toxic behaviors make them draining and stressful to work with for other developers.

Helpful behavior

  • use questions or recommendations to drive dialog - Never make demands for people to implement changes or ask judgmental questions, because this does not open up a dialog between you and the person whose code you are reviewing. Instead, ask what the review seeker thinks of your proposed solution.

  • collaborate, don’t back-seat drive - When you pair-program with another person, you should be there to ask questions, debate, and point to resources.

  • respond to every comment - If you as a review seeker don’t plan to apply a person’s feedback, just leave a note letting them know. Don’t ignore those who take time to help you.

  • know when to take a discussion offline - After dozens of contrasting PR comments and proposed solutions, it should be clear that online communication has become unproductive for the issue at hand. Send a meeting hit to involved members to discuss the issue offline. This way, your group can come to a decision more quickly and apply the solution.

  • use opportunities to teach, and don’t show off - Is your comment helping the other developer learn or are you nitpicking to participate?

  • don’t show surprise - Be careful not to show surprise that someone doesn’t have the same knowledge that you do on a topic. Being comfortable admitting you lack experience with a topic is a great way to ask for help.

  • automate what can be - Reviewing issues that can be caught by linters, git hooks, or automated tests are unhelpful because they often result in an avalanche of comments and come off as nitpicking. People are not particularly good at catching these issues, which is why automation tools exist.

  • refuse to normalize toxic behavior - Don’t join in and contribute to code review toxicity because it seems like the status quo, a hazing ritual for new developers, or just the way things are.

Last updated

Was this helpful?