Google released a Google engineering practice document the other day. In addition, the content of the document is all related to code review, including the content of how Google engineers conduct code review and the code Review guide. Github. IO /eng-practic…

The noun explanation of this article:

• cr: code review
• Cl: Change list refers to this change
• Reviewer: The reviewer for CR
• Nit: Nitpick
• Author: the developer of this CL, referred to as author in the original text. In foreign thinking, it means “author of CL”

If the article is too long, you can go straight to the end of the summary

The standard of cr

The main purpose of CR (Code Review) is to ensure that Google’s Code base is getting better and better. All related tools and processes are designed for this purpose. Achieving this will require a series of trade-offs

First, developers must be able to make progress on the tasks they are responsible for. If you never commit improved code to the code base, the code base will never improve. In addition, if a Reviewer makes cr difficult, developers will be reluctant to make improvements in the future.

On the other hand, it is the reviewer’s responsibility to ensure the quality of each Change List (hereinafter referred to as CL) and ensure that the overall code quality of its code base does not deteriorate. This can be tricky because often over time the code needs to degrade to make it work, especially when the team is under severe time constraints and people feel they have to cut corners to achieve their goals.

In addition, the Reviewer has ownership and responsibility for the code they are reviewing. They want to make sure the code is consistent, maintainable, and as mentioned in “What you can Get in CR” below.

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

In general, when a CL exists, a reviewer should approve it, and it will definitely improve the overall code quality of the working system, even if the CL is not perfect. This is the first principle in all CR. There are limits, of course. For example, if a CL adds a feature that a reviewer does not want to use in its system, then of course the reviewer can reject it, even if the code is well designed.

The key point here is that there is no “perfect” code, only better code. A reviewer should not require the author to retouch every little paragraph of an article before approving it. Instead, the Reviewer should weigh the need for development against the importance of their proposed change. Reviewer should not strive for perfection, but for continuous improvement. CL that improves the maintainability, readability, and understandability of the system as a whole should not be delayed for days or weeks just because it is not “perfect.”

A reviewer should always be available to leave a comment that says something better, but if it’s not critical, prefix the comment with a word like “nit:” to let the author know it’s just a modifier point they can choose to ignore.

To guide the

Cr has an important function to teach developers something new about languages, frameworks, or general software design principles. It’s ok to leave comments that help developers learn new things. Sharing knowledge is part of improving the health of your system’s code over time. You just need to remember that if your comment is purely educational but not essential to meeting the standards described in this document, please prefix it with “nit:” or otherwise indicate that the author does not have to address it in this CL.

The principle of

Reality and data override personal preference. When it comes to code style, the Style Guide is the absolute authority. Any points that are not in the Style Guide (Spaces, etc.) are a matter of personal preference. The code style should be consistent with the existing one. If the project doesn’t have the same style as before, embrace the author’s style.

Every aspect of software design has never been purely a matter of code style or personal preference. They are based on fundamental principles and should be based on them, not just on personal opinions, sometimes with little choice. If the author can demonstrate (through data or some fact based on principle) that his method is equally valid, the reviewer should accept the author’s preference. Otherwise, the choice of code style depends on standard principles of software design.

If no other rules apply, the Reviewer may require the author to be consistent with what is currently in the code base, as long as the code does not deteriorate the overall code health of the system.

Resolve the conflict

In any cr conflict, the first step should always be for the developer and Reviewer to try to reach a consensus based on this article and the CL Author’s Guide.

When it becomes particularly difficult to reach a consensus, a face-to-face meeting between reviewer and author is required, rather than just trying to resolve the conflict through CR’s annotations. (However, if you do, be sure to document the results of the discussion in CL comments for future readers.)

If this does not solve the problem, the most common solution is to upgrade. Often, the path to escalation is to have a broader team discussion, involve the team leader, ask the code maintainer for a decision, or ask the technical manager for help. Don’t let someone else sit on the sidelines because the author and reviewer can’t agree.

What to look for in CR

Note: When considering each point, it is important to consider the cr criteria.

design

What’s important in CR is to look at the overall design of CL. Does the interaction between different code segments in CL make sense? Does this change belong to your business code base or to some other code base imported? Does it integrate well with the rest of the system? Is this the right time to add this feature?

function

Did the CL do what the developer wanted? Does the developer benefit the user for whom the code was designed? The “users” are often both end users (when they are affected by changes) and developers (who must “use” the code in the future).

In most cases, we want developers to test CL sufficiently to make it work when they do CR. However, as a Reviewer, you should still consider edge situations, look for problems, try to think like the user, and make sure you don’t see errors just by reading the code.

If you want, you can verify CL yourself. Verifying CL changes is critical if the changes will directly affect the user’s visibility, such as UI changes. When reading the code, it can be difficult to understand how certain changes will affect users. For such changes, if it’s not convenient to test them yourself, you can have developers demonstrate the feature (demo).

Also, particularly important to consider functionality during CR is whether parallelism is programmed in CL, which could theoretically lead to deadlocks or race conditions. These types of problems are difficult to find by running code, and usually require careful consideration by someone (developer and Reviewer) to ensure they don’t introduce problems. (Note that this is also a good reason not to use parallel programming, in which case race conditions or deadlocks can occur that make it very complicated to examine or understand the code.)

complexity

Is a CL more complex than expected? For CL at any level, there are several things to check: Is a single line of code too complex? Is the function too complex? Is class too complex? “Complex” usually means that the code is hard to read, and probably also means that other developers have introduced other bugs while fixing the code

One type of complexity is the result of over-engineering, where developers make that code too generic than it should be, or add features that the system doesn’t currently need. Reviewer should pay special attention to overdesign. Encourage developers to solve problems that they know need to be solved now, rather than speculate about problems that might need to be solved in the future. Work on future problems when they arise, because you can see them more clearly

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

test

Think of unit testing, integration testing, and end-to-end testing as appropriate changes to require CL. In addition to production environment business code, tests should also be added to CL. Unless the CL exists to deal with something urgent.

Also, make sure your tests are correct, reasonable, and useful. Tests are not for their own sake, and they are rarely tested for their own sake (e.g., testing code to see if it has any problems and following the testing process), so we want to ensure that tests are effective.

Does the test fail when the code does have a problem? Does the test generate false positives if the program under test changes? Does each test make a simple and useful assertion? Are tests properly separated between different test methods?

Remember that test code is also code that must be maintained, not because it is not your primary concern.

named

Did the developers pick a proper name for everything? A good name means that the name is enough to fully express what something does or does. But at the same time, the name should not be too long to read

Refer to Clean Code 101 — Meaningful Names and Functions

annotation

Does the developer leave clear comments in understandable English? Are these comments really necessary?

Comments are usually useful when explaining why the code exists, not what the code is doing. If the code itself doesn’t make sense, that means it needs to be simplified even more. There are exceptions, of course, where comments explaining what the code is doing can be useful when explaining what a formal expression or complex algorithm is doing. But for the most part, comments are used to explain information that is not included in the program itself, such as the reason for doing it this way

It’s also helpful to look at previous comments for the CL, perhaps a Todo project is now available, a comment suggesting why not make this change, and so on.

Note that comments are different from class, module, and function files. The last three should be able to express the purpose of a piece of code, how to use it, and what to do when using it

style

Google has style guides for all the major languages, even most of the less popular ones, so make sure CL follows the instructions in the proper guides.

If you want to improve something in CL that is not included in the style guide, please preface your comments with Nit: to let developers know that this is a minor issue that you think could improve the code and is not mandatory. But remember, don’t block the CL submission based on your personal style preference alone.

Developers should not include major style changes along with other code changes in the CL, as this will make it difficult to see what changes the CL has made. It also makes merging and rolling back more complicated and creates other problems. For example, if authors want to reformat their code, let them refactor the new format into a new CL.

The document

If CL changes build, test, interaction, or release, check to see if it also updates related documentation, including README, g3doc pages, and other generated reference files. If CL removes or deprecates some code, consider whether the corresponding document should be removed and ask if it is missing.

Every line of code

Review each line of code you are assigned. Some things, like data files, generated code, and large data structures, you can scan a little. Never scan a class, function, or block of code written by a developer and assume that everything inside is fine. Obviously some code needs to be reviewed more carefully than others. This is a judgment call that must be made by you, but you should at least make sure that you understand what all the code is doing.

If reading code is too complex and slows down review, let the developer know and wait for them to explain the code before you review it again. We employ a lot of great software engineers at Google, and you’re one of them. If you don’t understand it, chances are no one else will. So when you ask developers to explain the code, you’re helping future developers understand it.

If you understand, but do not feel qualified to review a section, please be sure that a reviewer is a suitable (qualified) person to review the section. Especially for complex issues such as security, concurrency, accessibility, and internationalization.

context

It is often helpful to look at CL in sufficient context. Typically, CR tools only display a few lines of code around the modified section. But sometimes you have to look at the entire file to make sure the changes make sense. For example, you might only see four lines of new code added, but when you look at the entire file, you’ll see that those four lines were added to 50 lines of code, which you’ll need to break down into smaller functions.

It’s also useful to think about CL in terms of the whole system. Does the CL improve the code quality of the overall system, or does it make the system more complex? Is there a lack of testing? Never accept CL that degrades the code quality of the overall system. Because most systems are complicated by the accumulation of many small changes, it is also important to prevent new changes from introducing complexity, however small.

advantages

Let the developer know when you see something good in CL, especially if they address your comments in a great way. CRS usually focus only on errors, but they should also provide encouragement and appreciation for good practices. This is especially important when coaching developers: Rather than telling them what they’re doing wrong, tell them what they’re doing right.

Personally, I don’t think it’s about not pointing out mistakes, it’s about encouraging instead of pointing out mistakes, giving other developers more incentive to want to do things right. Let the other person know what they did well by simply saying something that they will continue to do and have a positive impact on other developers in the future

conclusion

When cr, be sure to:

• The code is well designed
• Functionality is good for users
• Make any UI changes reasonable and good-looking
• Any parallel programming implementation is secure
• Code should not be more complex than it needs to be
• Developers should not implement a feature they don’t use now that they may need in the future
• The code has appropriate unit tests
• The tests are well designed
• Developers use clear, unambiguous names for everything
• Comments should be clear and useful, and should only explain why, not what
• Code has proper write-down files (typically in G3doc)
• Code style conforms to style Guide
• Make sure you review every line of code you’re asked to review, verify the context, make sure you’re improving the quality of your code, and praise the good things the developers have done.

How to browse CL

Now that you know what to look for when reviewing, what is the most effective way to review changes spread across multiple files?

1. Are the changes reasonable? Does he have a good description?
2. Look at the most important changes to CL first. Does it have a good overall design?
3. Look at the remaining changes to CL in a reasonable order

Step 1: Take a macro view of the change and look at the CL description and what does it do

Does the change make sense and make sense? If you don’t think the change should happen in the first place, explain why it shouldn’t happen immediately. When rejecting changes like this, it’s also a good idea to advise developers on what they should do.

For example, you could say, “It looks like you’ve done something nice, thanks! But we’re actually moving in the direction of removing the FooWidget system you’re modifying, so we don’t want to make any new changes to it at this point. How about refactoring our new BarWidget class?”

It should be noted that the Reviewer should provide an alternative while politely rejecting the CL, and still be polite. This politeness is important because we want to show respect for each other even when we disagree.

If you have several CL’s that contain changes that you don’t want to make, you should either rethink the development team’s development process or release the development process to external contributors for more communication before any CL’s are written. It is better to say “no” before they start to invest, lest work that has already been invested must now be abandoned or completely rewritten.

Offer alternatives that let the other person know what to do instead of letting them figure it out on their own.

Step 2: Check the main part of CL

Find the files that are at the core of the CL. There are usually files in CL that contain a lot of logical changes, which is the main part of CL. So let’s look at the main parts first. This helps to provide the appropriate context for other, smaller parts of the CL, and it generally speeds up review. If the CL is too large to determine where the main part is, ask the developer what to look at first, or ask them to split the CL into multiple CLS.

If you find some major design problem in the main section, leave a comment immediately informing you of the problem, even if you don’t have time to look at the rest of the CL immediately. Because in fact, if the design problem is serious enough, continuing to review the rest of the code may be a waste of time, because the rest of the code under review may not be relevant or disappear.

It’s important to send critical design reviews right away for two main reasons:

• Usually, after a CL is sent out, developers are already working on new work based on that CL while waiting for a review. At this point, if the CL being reviewed has major design issues, the developer will have to rewrite all subsequent CL based on it. So you want to stop them before you invest in their problematic designs.
• Major design changes usually take a long time to complete, but almost every developer has their own deadline. In order to meet the deadline and ensure code quality, developers need to start or redo any major design changes as soon as possible.

Step 3: Look at the remaining CL changes in a reasonable order

Once you have confirmed that there are no major design issues with the entire CL, try to find a logical order to review the remaining files and make sure you don’t miss any of them. It is usually easiest to browse through each file in the order provided by the CR tools after you have reviewed the main sections. Sometimes it’s helpful to read the test before you read the main code so you know what to do and what to look for.

The speed of the review

Why is review faster

At Google, we optimize the speed of development teams working together to produce products, not the speed of individual development. The speed of individual development is important, but not as important as the speed of the team as a whole. When CR is slow, the following things happen:

• Team speed decreases. If review is slow, new features and bug fixes that are important to the rest of the team can be delayed for days, weeks, or even months because they are being reviewed or waiting for review.
• Developers began protesting cr. If a Reviewer responds only once every few days and each time requires a major change to the CL, the developer may feel very frustrated and difficult, and this often turns into a complaint. If a reviewer requested the same substantive change (and did improve the code quality situation), but was quick to respond each time a developer submitted a new change, these complaints would often go away. Most complaints about CR can often be addressed by speeding up the process.
• Code quality will suffer. When review is slow, there is increasing pressure on developers to submit CL that is not entirely satisfactory. Slower reviews also prevent others from cleaning up code, refactoring it, or even making further improvements to existing CL.

How fast should CR be?

If you are not at a time when you need to focus on your work, you should review the CL as soon as possible after it is submitted. The maximum limit is one working day. Following the guidelines above means that the average CL should be reviewed multiple times a day (if necessary).

Speed vs interruption

But sometimes individual speed takes precedence over team speed. Don’t interrupt yourself to work on CR if you need to focus on work (like writing code).

Studies have shown that it takes a long time for developers to get back into the flow after an interruption. Therefore, interrupting yourself during development is actually more expensive than having another developer wait for review.

Instead, we can find a good time to do cr before committing ourselves to the review review given by others. This could be after your current development task is finished, after lunch, after a meeting, after a mini-kitchen, etc.

Respond quickly to

When we talk about the speed of CR, we focus on response time, not how long CL takes to complete and submit. Ideally, the whole process should be quick.

In general, the speed with which individuals respond to comments is more important than getting the cr process to a quick end. Even if it can sometimes take a long time to complete the process, a quick reviewer response along the way will greatly reduce developer frustration with a slow CR process.

If you are too busy to do a full review of the CL, you can still quickly respond to let the developer know when you will be reviewing it, suggest a quick response with someone else (s), or provide some preliminary extensive comments. (Note: This does not mean you should interrupt development to reply — please find the appropriate interruption point to do so)

It is important that the Reviewer spend enough time reviewing to ensure that their LGTM means “this code meets our standards.”

Still, the ideal individual should respond as quickly as possible.

Across time zones review

When faced with different time zones, try to respond to the author while they are still in the office. If they’ve already gone home, make sure it’s done before they return to the office the next day.

LGTM comments

To speed things up, LGTM or Approval may be given by reviewer in some cases, even if there are still unresolved comments on CL. A similar situation can occur when:

• Reviewer is confident that the developer will properly process any remaining comments
• The rest of the comments are trivial or they don’t need to be handled by the developer
• Reviewer must clearly indicate which of the above situations they refer to

LGTM comments are especially worth considering when both parties are in different time zones, otherwise developers will wait all day to get “LGTM, Approval”.

Big changes

If someone asks for a reiveW, but the change is so large that you are not sure when you will have time to review it, what you should usually do is ask the developer to break up the CL into several smaller CL’s, rather than review the large CL at once. This can happen and be very helpful with reviewer (s), even if it requires additional developer effort to process.

If CL can’t be broken down into smaller ones, and you don’t have enough time to look at the whole CL quickly, at least write a comment about its overall design and send it back to the developers for improvement. As a Reviewer, one of your goals is to avoid slowing developers down or enabling them to quickly take other further actions without sacrificing code quality.

Cr’s capabilities will improve over time

If you follow these guidelines and are very strict with CR, you will find that the whole CR process will get faster and faster later. Because developers learn what good code is and deliver a great CL at the beginning, which happens to take less and less time. With a reviewer learning to respond quickly, rather than adding unnecessary delays to the process. But don’t compromise cr standards and code quality for the sake of speed, because it won’t actually make anything happen faster in the long run.

An emergency

In some emergency situations, CL may wish to relax standards in order to move quickly through the CR process. But look at what is an emergency to see what is actually an emergency and what doesn’t.

How to write a review

How to deal with delayed reviews

Sometimes developers defer processing comments generated by CR. Either they disagree with your suggestions or they will complain that you are too strict.

Who is right

When developers disagree with your proposal, take a moment to consider whether they are right. Because they usually know the code better than you do, they may actually have a better insight into some aspects of the code than you do. Does their argument make sense? Does it make sense from a code quality perspective? If so, let them know they’re right and let the problem sink in.

But developers aren’t always right. In this case, the Reviewer should go further and explain why she thinks her suggestion is correct. A good explanation usually shows “understanding of the developer’s response” and information about why changes were requested. In particular, if a reviewer believes a suggestion will improve code quality, continue to pitch your case. As long as they believe that the extra work required will ultimately improve overall code quality. Improving overall code quality often happens with every minor change. Sometimes it takes several explanations to get a suggestion across. Remember, always be polite and let the developer know that you hear what they are saying, but you just don’t agree with the argument.

Frustrate developers

Reviewer may sometimes think that if you insist on making improvements, it will frustrate the developer. It’s true that developers sometimes get frustrated, but it’s usually short-lived and even later they thank you very much for helping them improve their code. In general, as long as you are polite in your comments, the developer won’t actually be upset at all; these concerns are only a reviewer’s concern. Frustration is often related to the way cr reviews were written, rather than a reviewer’s insistence on code quality.

Come back later and clean it up

A common reason for delay is that developers (understandably) want to complete tasks. They don’t want to approve this CL through another round of CR. At this point, they’ll usually say that it’s going to be sorted in the later CL, so you should give it to LGTM now. Of course, some developers are very good at this and immediately send out a follow-up CL that fixes the problem, but this follow-up “cleanup behavior” is very rare based on past experience. This would not have happened unless the developer had cleaned up immediately after the CL was approved. This is not because developers are irresponsible, but because they may have so much other work to do that the cleanup gets lost in the pile. Therefore, it is usually best to insist that developers clean up code after merging it. Because having people “clean up later” is the most common way to degrade the quality of your code base.

If CL introduces new complexity, it must be handled before committing unless it is an emergency. If CL causes surrounding problems to be exposed and cannot be resolved now, developers should document and assign defects to themselves to avoid subsequent oblivion. Or they can choose to leave a TODO comment in the code and link to the defect they just logged.

Common complaints about the rigor of reviews

If you do CR with fairly loose standards and move to stricter standards, some developers will start complaining loudly. Generally speaking, increasing the speed of review will make these complaints fade away. It may take a few months for these complaints to go away, but eventually developers will see the value of strict review because it helps them produce good code. And when something happens, the loudest protester may even be your strongest supporter because they see the value in being review strict.

Resolve the conflict

If you follow all the previous steps but still encounter a conflict between two parties that cannot be resolved, refer to the cr standards above for guidelines and principles that can help resolve the conflict.

The translator to summarize

The standard of cr

• The Reviewer will be responsible for ensuring the quality of CL as the owner of the code for reiVEW
• Reviewer should not strive for perfection, but for continuous improvement
• Cr is instructive
• The code style should be consistent with the existing one. If the project doesn’t have a uniform style, embrace the author’s style
• When it is difficult to reach a consensus to resolve conflicts, it is necessary to have face-to-face discussions or engage a larger team with the leader

What to look for in CR

• Overall design of CL
• Functional validation, functionality is good for users, and any UI changes should be reasonable and good-looking
• Is it complicated? Is it over-engineered
• The code has appropriate unit tests
• The tests are well designed
• Normative naming, you know what the name is, right
• The appropriate comment, the comment should be why not what
• Code style follow style Guide, if need to change code style should be solved in another CL
• CL updates documentation as it changes builds, tests, interactions, and releases
• Review every line of code (except resource files, generated code, large data structures) carefully. Let the developer explain if it’s too complicated
• Security, concurrency, accessibility, internationalization and other complex issues need to be reviewed by the right people
• Think of CL in terms of the whole system
• When reviewing, instead of telling developers what they did wrong, tell them what they did right

How to browse CL

• To take a macro view of the change, look at the CL description and what does it do
• Check the main part of CL
• Look at the remaining changes to CL in a reasonable order

The speed of the review

• Slow reviews will slow down the team as a whole, developers will start protesting cr, and code quality will suffer
• Don’t interrupt yourself to work on CR if you need to focus on work (like writing code).
• The speed with which individuals respond to comments is more important than bringing the cr process to a quick end
• When faced with different time zones, try to respond to the author while they are still in the office
• To speed things up, LGTM or Approval may be given by reviewer in some cases, even if there are still unresolved comments on CL
• When the change is too large to review, the usual thing to do is to ask the developer to break up the CL into smaller ones
• Cr speeds should get faster and faster, but don’t compromise CR standards and code quality for the sake of speed

How to write a review

• When developers disagree with your suggestion, think about who is right, explain it clearly, and be polite.
• If Review sticks to its guns, it will frustrate developers. Frustration is often related to the way the CR review was written, not the reviewer’s insistence on code quality.
• If CL introduces a new problem, it must be resolved before committing unless it is an emergency.
• If the problem with the review cannot be resolved now, the TODO comment and link to the defect just documented.

What if review is too strict

Increasing the speed of review will make these complaints disappear. It may take a few months for these complaints to go away, but eventually developers will see the value of strict review because it helps them produce good code.

Pay attention to the public account “Different front-end”, learn front-end from a different perspective, grow fast, play with the latest technology, explore all kinds of black technology