codereview

Posted on Edited on In Waline:

During the few days when everyone was happy, Google released two docs, which I feel is very necessary to know as a programmer and engineer. After reading, the author intercepted some of the original text and simplified some other parts to summarize.

The

Simply put, it is the CL author guide to guide through code review. So what does CL mean? It means change list, change record and log.

This guide can help you complete code reviews faster and obtain higher quality results. For every Google developer, reading this guide is very helpful.

https://google.github.io/eng-practices/review/developer/

Writing

First

  • Short summary of what is being done.
  • Complete sentence, written as though it was an order.
  • Follow by empty line.

First of all, the first line of the CL should preferably be a short description of what was done, followed by a blank line, and second, it should be written as imperative, of course the following part may not need to be written like this.

Body

The rest of the description should be informative. It may include a brief description of the problem to be solved and why this is the best approach. If there are any shortcomings in the approach, it should be mentioned. If relevant, please include background information such as error numbers, benchmark results, and a link to the design doc…

Bad

“Fix bug” is an inadequate CL description. What bug? What did you do to fix it? Other similarly bad descriptions include:

  • “Fix build.”
  • “Add patch.”
  • “Moving code from A to B.”
  • “Phase 1.”
  • “Add convenience functions.”
  • “kill weird URLs.”

Some of those are real CL descriptions. Their authors may believe they are providing useful information, but they are not serving the purpose of a CL description.

Good

Here are some examples of good descriptions.

Functionality

rpc:

Servers

The first few words describe what the CL actually does. The rest of the description talks about the problem being solved, why this is a good solution, and a bit more information about the specific implementation.

There are many good examples of CL in the doc, you can take a look for yourself. At the same time, remember that CL is preferably all English.

Small

Why

  • Reviewed more quickly. It’s easier for a reviewer to find five minutes several times to review small CLs than to set aside a 30 minute block to review one large CL.
  • 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 CL and see if a bug has been introduced.
  • Less wasted work if they are rejected. If you write a huge CL 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 CL 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 CL in review.
  • Simpler to roll back. A large CL will more likely touch files that get updated between the initial CL submission and a rollback CL, complicating the rollback (the intermediate CLs will probably need to be rolled back too).

Simply put, it is easier to know what you are doing with each commit, so that when you have a problem, you can quickly locate the problem, and if the problem needs to be rolled back, it will not be affected by the existing code.

What

In general, the right size for a CL is one self-contained change. This means that:

  • The CL makes a minimal change that addresses just one thing. This is usually just one part of a feature, rather than a whole feature at once. In general it’s better to err on the side of writing CLs that are too small vs. CLs that are too large. Work with your reviewer to find out what an acceptable size is.
  • Everything the reviewer needs to understand about the CL (except future development) is in the CL, the CL’s description, the existing codebase, or a CL they’ve already reviewed.
  • The system will continue to work well for its users and for the developers after the CL is checked in.
  • The CL is not so small that its implications are difficult to understand. If you add a new API, you should include a usage of the API in the same CL so that reviewers can better understand how the API will be used. This also prevents checking in unused APIs.

For the size of each commit, about 100 lines per commit is a reasonable size, and 1000 lines is too big. But this is not an absolute criterion. If 100 lines of code are distributed in 50 files, it is also a big modification.

When

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 CL has been generated by an automatic refactoring tool that you trust completely, and the reviewer’s job is just to sanity check and say that they really do want the change. These CLs can be larger, although some of the caveats from above (such as merging and testing) still apply.
Splitting

Relatively independent changes that require different reviewers can be split into different CLs

Separate

It is usually best to refactor in a separate CL without including feature changes or bug fixes. For example, modifying a class and calling it should be placed in two different CLs.

Keep

Try not to put the test code in different CLs, because the test will ensure the correctness of your code, even if the test code may make your submission huge.

However, we can put independent tests into a separate CL, for example:

  • validating pre-existing, submitted code with new tests.
  • refactoring the test code (e.g. introduce helper functions).
  • introducing larger test framework code (e.g. an integration test).
Don’t

If multiple CLs will depend on each other, it is best to ensure that the system can operate normally after each CL is submitted for modification.

Can’t

How

When you’ve sent a CL out for review, it’s likely that your reviewer will respond with several comments on your CL. Here are some useful things to know about handling reviewer comments.

Don’t

The goal of review is to maintain the quality of our codebase and our products. When a reviewer provides a critique of your code, think of it as their attempt to help you, the codebase, and Google, rather than as a personal attack on you or your abilities.

In other words, try not to add your own negative emotions to the code review process. If someone adds negative emotions to your code review process, first consider what they really want to say, and then find a way to communicate with them privately, such as an interview or email.

Fix

If the reviewer doesn’t understand your code, then you should find a way to explain what your code is doing. Because your reviewer can’t understand it, other colleagues may not understand it in the future.

Think

Because we may spend a lot of time on each submission, when we see reviewers give us comments, our first reaction is to reject.

But we’d better first consider whether the reviewer’s opinion is correct. If we can’t answer this question, we may need to find the reviewer to make his point clear, and then judge.

Resolving

Your first step in resolving conflicts should always be to try to come to consensus with your reviewer. If you can’t achieve consensus, see The Standard of Code Review, which gives principles to follow in such a situation.

How

This doc contains advice on the best way to conduct code reviews. It is a very complete doc, divided into many separate sections. Reading this doc will definitely be very helpful for everyone, and it is definitely one thing that will benefit everyone a lot.

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

The

The main purpose of code review is to ensure that the overall code health of Google’s codebase continues to improve over time.

First of all, developers must be able to make progress on their tasks. If you have never submitted improvements to the codebase, the codebase will never be improved. Also, if it is difficult for reviewers to make any changes, then developers have no incentive to make improvements in the future.

On the other hand, the reviewer is responsible for ensuring that the quality of each CL is such that the overall code health of its codebase does not decrease over time.

Thus, we get the following rule as the standard we expect in code reviews:

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

That is the senior principle among all of the code review guidelines.

Mentoring

Code review has an important function that can teach developers new knowledge about languages, frameworks, or general software design principles. It is always good to leave comments that help developers learn new knowledge. Sharing knowledge is part of improving the performance of system code 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

Design

The most important content in the review is the overall design of CL. Does the interaction of various codes in CL make sense?

Functionality

Does this CL achieve the intended purpose of the developer? What benefits does the developer intend to bring to the users of this code?

“Users” are usually both end users (when they are affected by changes) and developers (who will have to “use” this code in the future). Usually, we want developers to test the CL well to ensure that they work properly when doing code reviews.

However, as a reviewer, you should still consider edge cases, look for concurrency issues, try to think like a user, and ensure that there are no errors that you can see just by reading the code.

You can verify the CL as needed, and the most important time for reviewers to check the behavior of the CL is the impact on users (such as UI changes).

When you are only reading the code, it is difficult to understand how certain changes will affect users. When making such changes, if it is too cumbersome to patch in CL and try it yourself, you can let the developer demonstrate the feature to you.

Another time when it is particularly important to consider functionality during code review is whether there is some kind of parallel programming going on in the CL that could theoretically lead to deadlocks or race conditions. Such issues are difficult to detect just by running the code, and usually require someone (developers and reviewers) to carefully consider them to ensure that they do not introduce problems.

Complex

Simply put, don’t overdesign.

Tests

Unit Test, Integration Test, or E2E Test as required by the change. Typically, tests should be added in the same CL as the production code unless the CL handles an emergency. Make sure the tests in the CL are correct, reasonable, and useful. Tests don’t self-test, we rarely write tests for tests - people have to make sure the tests work.

Naming

The name should be long enough until he can fully express what it is or what it wants to do.

Comments

Does the developer write clear comments in understandable English? Are all comments actually necessary? Usually, comments are useful when they explain why certain code exists and should not explain what certain code is doing. If the code is not clear enough to explain itself, the code should be simplified. There are some exceptions (for example, Regular Expression and complex algorithms often benefit greatly from comments explaining their role), but most comments are for information that the code itself may not be able to contain, such as the reason behind the decision.

Style

Make sure the CL follows the appropriate style guide. If you want to improve a style point that is not in the style guide, preface the comment with “Nit:” to let the developer know that this is an option that you think can improve the code but is not mandatory.

Authors of CL should not combine major style changes with other changes. This makes it difficult to see changes in CL, makes merging and rollback more complicated, and causes other issues.

Documentation

If CL changes the way users build, test, interact with code, or release code, please check if it also updates the relevant doc, including the readme file

Every

Look at every line of code that has been assigned to you for inspection. Sometimes you can scan data files, generated code, or things like large data structures, but not manually written classes, functions, or Code Blocks and think what’s in them is okay. Obviously, some code needs to be scrutinized more than others - this is a call to judgment you have to make - but you should at least make sure you understand what all the code is doing. If you are having too much difficulty reading the code and are making the review slow, then you should let the developers know this and wait for them to clarify before attempting the review

Context

Usually, the code review tool will only show you a few lines of code around the part you want to change. Sometimes, you have to look at the entire file to make sure the changes really make sense.

Good

If you see something nice in CL, please tell the developer, especially when they answered one of your comments in an excellent way.

Summary

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.

Now that you know what to look for, what’s 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.

Speed

Why

At Google, we optimize for the speed at which a team of developers can produce a product together, as opposed to optimizing for the speed at which an individual developer can write code. The speed of individual development is important, it’s just not as important as the velocity of the entire team.

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 CL 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 CL 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 CLs that are not as good as they could be. Slow reviews also discourage code cleanups, refactorings, and further improvements to existing CLs.

How

  • Be kind.
  • Explain your reasoning.
  • Balance giving explicit directions with just pointing out problems and letting the developer decide.
  • Encourage developers to simplify code or add code comments instead of just explaining the complexity to you.