|Reprinted from: Development Notes of Chivalry Dream
|Edited by: Yuan Ruibin
|Designer: Tan Jialu
The main purpose of code review is to ensure that the overall code health of Google's code base continues to improve over time. All tools and processes for code review are designed for this purpose.
In order to achieve this, a series of trade-offs must be made.
First, developers must be able to make progress on their tasks. If developers never commit improvements to the code base, the code base will never be improved. In addition, if the reviewers do not make any changes, then the developers will have no incentive to improve later.
On the other hand, reviewers are responsible for ensuring the quality of each CL (change list), so that the overall code health of its code base will not decrease over time. This can be tricky, because over time, the code base usually degrades due to a small decline in code performance, especially when the development team is pressed for time and thinks that it must take shortcuts to achieve its goals.
In addition, reviewers are responsible for the code they are reviewing. Ensure that the code base is consistent and maintainable, as well as other matters mentioned in "What to look for in code review".
Therefore, we take the following rules as the expected standards in code review:
Generally, even though CL is not perfect, if it can improve the overall code quality of the system, the reviewer can approve it.
This is the highest principle in all code review guidelines.
Of course, there are exceptions. For example, if CL adds functions that reviewers do not want to use in their systems, reviewers can definitely refuse to approve even if the code is properly designed.
The key point here is that there is no "perfect" code, only better code. Reviewers should not require developers to polish every small detail of CL before approval. The reviewers should pursue continuous improvement rather than perfection. In general, if a CL can improve the maintainability, readability and understandability of the system, it should not be delayed for days or even weeks because it is not "perfect".
The reviewers should provide suggestions at any time, so that the developers may do better. But if it is not very important, you can add words like "Nit:" before it, so that the developers know that it is only an improvement suggestion, and they can choose to ignore it.
Code review has an important function, which can teach developers new knowledge about language, framework or general software design principles. Sharing knowledge over time will become part of improving the running quality of system code. But please note that if your suggestions are purely educational and are not so important for meeting the standards described in this article, please add "Nit:" in the front or tell developers in other ways that they do not have to solve these problems in CL.
-
Objective technology and data are more important than personal opinions and preferences.
-
In terms of code style, the style guide is the absolute authority. Any pure style points (spaces, etc.) that are not in the style guide are personal preferences. The style should be consistent with the style there. If there is no previous style, please accept the author's style.
-
Software design has never been a pure style issue, nor personal preference. They are based on fundamental principles and should be weighed against, not just individual opinions. Sometimes there are some effective choices. If the developer can prove (through data or based on reliable engineering principles) that several methods are equally effective, then the reviewer should accept the choice of the developer. Otherwise, decisions should be made based on the principles of software design standards.
-
If there are no other applicable rules, the reviewer can ask the developer to keep consistent with the content in the current code base, as long as the overall code operation of the system does not deteriorate.
In case of conflict in code review, the first step should always be to make developers and reviewers reach a consensus based on this document and other documents in the CL Author's Guide and Reviewer's Guide.
When reaching a consensus becomes particularly difficult, it may be helpful to have a face-to-face meeting or VC between developers and reviewers, rather than just trying to resolve conflicts through code review comments. (However, if you do, make sure to record the results of the discussion in the comments on CL for future reading.)
If you can't solve the problem, you should consider upgrading it. The way to upgrade is usually to have a broader team discussion, including involving the team leader, asking the code maintenance personnel to make a decision, or asking the engineering manager for help. Don't let CL hang there just because developers and reviewers can't reach an agreement.
Search content in code review
The most important part of code review is the overall design of CL. Is the interaction of various codes in CL meaningful? Does this change belong to the code base or to a package? Is it well integrated with the rest of the system? Is this a good time to add this feature?
Does this CL meet the developer's expectations? What benefits do developers intend to bring to users of this code? "Users" are often both end users (when they are affected by changes) and developers (they will have to "use" this code in the future).
Generally, we hope that developers can test CLs well to ensure that they work properly during code review. However, as a reviewer, you should still consider the marginal situation, look for concurrency problems, try to think like a user, and find errors that cannot be seen only by reading the code.
You can verify CL as needed. For reviewers, the best time to check CL behavior is when it has an impact on users, such as UI changes. When just reading the code, it's hard to understand how some changes will affect users. For such changes, if it is too troublesome to patch in CL and try by yourself, you can let the developers provide function demonstration.
Another opportunity to consider functionality during code review is whether some parallel programming is in progress in CL, which may theoretically lead to deadlocks or race conditions. It is difficult to detect such problems only by running code, and people (developers and reviewers) usually need to consider them carefully to ensure that no problems will be introduced.
Is CL more complex than what is actually needed? Check at each level of CL - are individual lines too complex? Is the function too complex? Class too complex? "Too complex" usually means "the code reader cannot quickly understand." It may also mean "the developer may introduce errors when trying to call or modify this code."
Overdesigning a special complexity, that is, developers make the code more general than the actual needs, or add functions that the system does not need at present. The reviewers should be particularly alert to over design. Encourage developers to solve known problems that they need to solve now, rather than solving problems that developers speculate may need to solve in the future. Future problems should be solved after they appear, because at that time we can see their actual situation and needs.
Conduct unit test, integration test or end-to-end test according to the change requirements. In general, unless the CL handles an emergency, you should add tests in the same CL as the production code.
Ensure that the tests in CL are correct, reasonable and useful. Testing cannot test itself. We seldom write tests for tests, so we must ensure that tests are effective.
Does the test fail when the code is broken? If the code changes below it, will they start generating false positives? Does each test make simple and useful assertions? Are tests properly separated between different test methods?
Remember that tests are also code that must be maintained.
Do developers use good naming practices? A good name should be able to fully express what an item (variable, class name, etc.) is or is used for, but it should not be too long to read.
Do developers write clear comments in understandable natural language? Are all comments necessary? Generally, good comments should explain why some code exists, rather than what some code is doing. If the code is not clear enough to explain itself, simplify the code. There are also some exceptions (for example, regular expressions and complex algorithms usually explain their functions in comments, which will benefit people who read code), but most comments are for information that may not be included in the code itself, such as the reasons behind these codes.
It is also helpful to review the comments before this CL. Maybe there is a to-do item that can be deleted now. It is suggested not to make this change.
Note that comments are different from the documents of classes, modules or functions. Comments should indicate the purpose of a piece of code, how it should be used, and the behavior when it is used.
Google provides code style guidelines for major programming languages and most minor programming languages to ensure that CL follows appropriate style guidelines.
If you want to improve the style points that are not in the guide, please add "Nit:" before the comment so that the developers know that this is an option that you think can improve the code but is not mandatory. But please do not block CL submission only based on personal preferences.
Developers should not combine major style changes with other changes. This makes it difficult to see the changes in CL, makes merging and rollback more complex, and causes other problems. For example, if the developers want to reformat the entire file, they should only send the reformatted format as a CL to the reviewers, and then send another CL with functional changes.
If CL changes the way users build, test, interact with code or release code, check whether it has updated relevant documents, including README, g3doc pages and other generated reference documents. If CL deletes or discards code, consider whether the document should also be deleted. If the document is missing, ask the developer for it.
Look at each line of code. Sometimes you can scan things such as data files, generated code or large data structures, but do not scan manually written classes, functions or code blocks, and assume that the contents can run normally. Obviously, some codes need to be examined more carefully than others - it's up to you to decide which codes are, but you should at least understand what these codes are doing.
If the code is complex or you can't quickly understand it, which will slow down the review, you should let the developers know this, wait for their explanation, and then try the review again. Google has hired excellent software engineers. If they cannot understand the code, other developers may not understand it. Therefore, requiring developers to explain can also help future developers understand this code.
If you understand the code but are not qualified for code review, please ensure that there is a qualified reviewer on CL, especially on complexity issues (such as security, concurrency, accessibility, internationalization, etc.).
Typically, the code review tool displays only a few lines of code that need to be changed. But sometimes you have to look at the entire file to make sure the changes really make sense. For example, you may see that only four lines of code have been added, but when you view the entire file, you will see that these four lines are in 50 lines of methods, which need to be broken down into smaller methods.
CL needs to be considered based on the whole system. Is this CL improving the code operation of the system, or making the whole system more complex and unpredictable? Do not accept CL that will reduce the health of system code. Most systems become complex through many small changes, so it is important to prevent small complexity in new changes.
If you see something good in CL, please tell the developers, especially when they answer your comments in an excellent way. Code reviews usually focus only on errors, but good practices should also be encouraged and appreciated. Sometimes it's more valuable to tell developers what's right than what's wrong.
When conducting code reviews, you should ensure that:
-
The code is carefully designed.
-
This function is useful for code users.
-
The UI changes are reasonable.
-
Any parallel programming is done safely.
-
The code complexity should not exceed the expected level.
-
It is not necessary to realize the requirements that may appear in the future.
-
The code has appropriate unit tests.
-
Well designed test cases.
-
Developers clearly name everything.
-
Clear and useful code comments should explain "why" rather than "what".
-
Appropriate code documentation.
-
This code conforms to our style guide.
Make sure to check every line of code, check the context, make sure to improve the code health, and praise the excellent work done by the developers.
After knowing what to focus on in code review, how to effectively conduct cross file code review?
-
Does it make sense to change the code? Does it have a good description?
-
First, let's take a look at the most important part of the change. How is the overall design?
-
View the remaining CLs in the appropriate order.
Step 1: Comprehensively understand code changes
View CL description and general functions of CL. Does this change make sense? If the change is unnecessary, please reply immediately and explain why the change should not be made. When you reject such changes, it's better to suggest to the developers what they should do.
For example, you may say: "It seems that you have done some excellent work for this, thank you! But actually, we are going to remove this system, so we don't want to make any new changes to it now. Perhaps, you can refactor our new BarWidget class?"
Please note that when the reviewer rejects the current CL, he/she should also make other suggestions and behave politely. This kind of politeness is very important, because as developers, even if we don't agree, we should show that we respect each other.
If multiple CLs are changes that you don't want to make, you should consider redesigning the team's development process or the published process of external contributors to communicate more before writing CLs. It's best to tell people "no" before they finish a lot of work that must now be thrown away or completely rewritten.
Step 2: Check the main parts of CL
Find files that belong to the Main section of this CL. In general, the file with the most logical changes is the main part of CL. First, look at these main parts. This helps provide context for all the smaller parts of the CL, and generally speeds up code reviews. If the CL is too large, you can't determine which parts are the main ones. Please ask the developers what you should look at first, or ask them to divide the CL into multiple CLs.
If you find that there are some major design problems in this part of the CL, you should immediately reply to the developers, even if you do not have time to review the rest of the CL. In fact, reviewing the rest of the CL can be a waste of time, because if the design problem is serious enough, many other code will become irrelevant.
There are two main reasons why developers should reply immediately:
-
Developers usually issue a CL, and then immediately start new work based on that CL while waiting for an audit. If there are major design problems in the CL you are reviewing, they will also have to redesign their future CLs. Therefore, it is better to tell developers before they spend unnecessary time on problematic designs.
-
Major design changes take longer than minor changes. In order to complete the work before the deadline and retain high-quality code in the code base, developers need to start all rewriting of CL as soon as possible.
Step 3: Check the remaining CLs in proper order
After confirming that CL has no major design problems as a whole, please try to find out the logical order to browse the files, and ensure that you do not miss the review of any files. Usually, after browsing the main files, it is easiest to browse each file in the order that the code review tool shows you them. Sometimes it is helpful to read the tests before reading the main code, because then you can know what changes should be made.
Why should code reviews be quick?
At Google, we have optimized the overall delivery speed of the development team (rather than the speed of writing code for individual developers). Individual development speed is also important, but its importance is not as important as the development speed of the whole team.
When code review is slow, the following things will happen:
-
The overall development speed of the team has decreased. If individual developers cannot respond quickly to reviews, it may be because they have other things to do. However, if each CL has to wait for review again and again, the new functions and bug fixes of the rest of the team will be delayed for several days, weeks or months.
-
Developers began to protest against the code review process. If the reviewer only replies every few days, but each time requires a major change to the CL, this may be very frustrating and difficult for the developer. Usually, this is expressed as a complaint about the "strictness" of the reviewers. If the reviewers can quickly provide feedback (changes that can really improve the code's health), the complaints will disappear, even if they ask for the same changes. In fact, most complaints about the code review process can be resolved by speeding up the review process.
-
The quality of the code may be affected. When the review speed is slow, the pressure of developers will also increase, because they cannot submit imperfect CLs. The slow review process also hinders code cleaning, refactoring, and further improvement of existing CLs.
How fast should code reviews be?
If you are not concentrating on the task at hand, you should review the code at the first time.
One workday is the longest time required to respond to a code review request (that is, the first thing in the morning of the next day).
Following these guidelines means that a typical CL should have multiple rounds of reviews in one day, if needed.
In one case, personal speed is more important than team speed. If you are dealing with important tasks such as writing code, please do not interrupt yourself to check the code. Research shows that it takes a long time for developers to recover to a stable development process after an interruption. Therefore, interrupting yourself while writing code is actually more expensive for the team than letting other developers wait a little while for code review.
Therefore, in this case, you can wait until your work can stop before starting code review. It can be after finishing the coding task at hand, after lunch, after the meeting, after the break, etc.
What we mean by code review speed is the response time, not the time taken by CL to pass the whole review process and submit. The whole process should also be fast under ideal conditions, but the response speed of a single review request is more important than that of the whole process.
Even if it sometimes takes a long time to complete the whole review process, getting quick response from reviewers during the whole review process can greatly alleviate developers' dissatisfaction with "slow" code review.
If you are busy conducting a comprehensive review on CL, you can still send a quick response first to let the developers know when to start the review, or suggest that other reviewers who can respond faster review the code, or provide some preliminary feedback.
It is important that reviewers spend enough time reviewing to ensure that the code meets the standards. However, it is better to respond faster.
Cross time zone code review
When dealing with cross time zone code reviews, try contacting developers while they are still in the office. If they have already gone home, it is best to ensure that they can see the code review completed when they return to the office the next day.
In order to speed up code review, in some cases, reviewers should still give LGTM/approval even if they leave unresolved comments on CL. Complete this operation in one of the following cases:
-
The reviewer is confident that the developer will handle the suggestions and comments given by the reviewer.
-
The remaining changes are minor and do not necessarily require developers to complete them.
Unless otherwise specified, reviewers should use one of these options.
When developers and reviewers are in different time zones, LGTM with comments is particularly worth considering, otherwise developers will wait for a whole day to obtain "LGTM, Approval".
If someone sends you a big code review, you are not sure when you can have time to review it. The general response should be to ask developers to split the CL into multiple smaller CLs that are built together. Instead of having to review all the huge CLs at once. This is usually reasonable and helpful to reviewers, even if developers need to do some extra work.
If a CL cannot be decomposed into smaller CLs, and you do not have time to quickly review the entire CL, then at least write some comments on the overall design of the CL, and then send it back to the developers for improvement. As a reviewer, one of your goals should be to respond quickly to developers without affecting code quality, or to enable them to take further action quickly.
Improvement of continuous code review
If you follow these guidelines and strictly implement code review, you should find that the whole code review process will become faster and faster as time goes by. Developers know what needs to be done to ensure code quality, and send you excellent CL from the beginning, so that the time required for review will be less and less. Reviewers learn how to respond quickly without adding unnecessary delays in the review process. However, don't compromise on code review standards or quality to improve speed -- in the long run, this won't make anything happen faster.
In some emergencies, CL must pass the whole audit process very quickly, and the quality criteria will be relaxed. However, see What is an emergency? Describe which situations can actually be regarded as emergencies and which cannot.
-
-
-
Make a trade-off between giving clear instructions and pointing out problems and letting developers decide.
-
Developers are encouraged to simplify code or add code comments, not just to explain complexity.
In general, it is important to be polite and respectful, but also to be very familiar with the developers who want to view their code. One way is to make sure that your comments are for the code, but for the developer. You don't always have to follow this practice, but you must use it when you say something that might be frustrating or controversial. For example:
Bad saying: "Why use threads in this place? Obviously, this will not gain any benefits"
Good saying: "The concurrency model increases the complexity of the system here, but I don't see any actual performance advantages. Since there is no performance benefit, it is better to use this code as a single thread rather than multiple threads."
As can be seen from the correct example above, this helps developers understand why these suggestions should be given. You may not always need to include this information in your comments, but sometimes the appropriate practice can become an understanding of your intentions, the best practices followed or your suggestions on how to improve code quality.
In general, it is the responsibility of the developer, not the reviewer, to fix the CL. You do not need to perform the detailed design of the solution or write code for developers.
However, this does not mean that reviewers should not help. In general, you should strike the right balance between pointing out problems and providing direct guidance. Pointing out problems and letting developers make decisions can often help developers learn and make code reviews easier. This can also lead to better solutions because developers are closer to code than reviewers.
However, sometimes it is more helpful to give instructions directly, suggestions or even codes. The main purpose of code review is to obtain the best CL. The second goal is to improve the skills of developers so that they will have fewer and fewer reviews later.
If you ask developers to explain a piece of code that you don't understand, they usually rewrite the code. Sometimes, adding comments to code is also an appropriate response, as long as it does not just explain overly complex code.
The instructions written only in the code inspection tool will not help future code readers. Only a few cases can accept this practice. For example, you are not familiar with the review, but many people are familiar with the developer's explanation.
Sometimes, developers push back code reviews. They may disagree with your suggestions or complain that you are generally too strict.
When developers disagree with your suggestions, please take a moment to consider whether they are correct. Often, they are closer to the code than you are, so they may actually have a better understanding of some aspects of the code. Does their argument make sense? Does this make sense from a code quality perspective? If so, please let them know that they are right, and then the problem will be solved.
However, developers are not always right. In this case, the reviewers should further explain why they think their suggestions are correct. A good explanation not only explains the understanding of developers, but also explains why they are required to change other information.
If the reviewers believe that their suggestions can improve the code quality and that the code quality improvement brought by the review is worth the extra work of the developers, they should stick to it. Improving code quality often consists of a series of small steps.
Sometimes, it takes a lot of time to explain before really making a proposal. Make sure you are always polite and let the developers know that you understand what they are saying, and you just disagree.
Reviewers sometimes think that if they insist that developers make changes, it will frustrate developers. Sometimes, developers do feel unhappy, but this is usually temporary, and then they will thank you very much for helping them improve code quality. If you behave politely, the developers will not feel unhappy at all. This worry may be unnecessary. Worries are usually related to the way reviewers write comments, rather than the reviewers' insistence on code quality.
A common reason to fall back is that developers want to get things done (understandably). They do not want to conduct another round of review just to obtain this CL. Therefore, they said that they would clean up some content in the CL in the future, so the reviewers should now LGTM this CL. Some developers are very good at this and will immediately write subsequent CLs to solve this problem. However, experience shows that the longer developers spend writing the original CL, the less likely they are to make subsequent repairs. In fact, unless the developer fixes the current CL immediately after submitting it, he or she will not do this after passing it. This is not because the developers are irresponsible, but because they have a lot of work to do, the repair work is usually forgotten. Therefore, it is best to insist that developers repair their CLs immediately before the code enters the code base and "finishes". Letting developers "fix it later" is a common way of code base degradation.
If CL introduces new complexity, it must be cleared before submission unless it is an emergency. If CL exposes some problems that cannot be solved now, developers should record the errors and assign them to themselves to avoid loss. They can also choose to write TODO comments in code that references committed errors.
Complain that the review is too strict
If your code review was not so strict before, but suddenly became strict, some developers will complain loudly. It doesn't matter, though, that improving the speed of code reviews usually makes these complaints disappear.
Sometimes, these complaints may take several months to disappear, but finally developers will see the value of strict code review, because they will see that strict code review helps them produce excellent code. Sometimes, if this happens, the strongest protesters will even become your strongest supporters.
If you follow all of the above, but still encounter unresolved conflicts during code review, please refer to the above again for guidelines that can help resolve conflicts.
Related reading | Related Reading
Yibai said open source, using the network effect of the To C era to build the To B basic software
Microsoft Build 2020 Developers Conference invites the Open Source Agency to take a new online journey
Apache License for Authentic Human Language
