How to conduct a code review
From Google's Engineering Practices documentation
This guide provides recommendations for optimizing code reviews based on years of experience. Together, they make up one document, divided into many sections. It is not necessary to read all of them, but often it is better for yourself and the team to study the entire manual.
See also the CL Author’s Guide for detailed advice on developers whose commits are reviewed.
The main purpose of a code review is to guarantee continuous improvement of the Google codebase. All tools and processes are dedicated to this goal.
A number of compromises are needed here.
First, developers should be able to successfully solve their problems . If you never send a code, then the code base will never improve. In addition, if the reviewer greatly complicates any work, then in the future developers are not interested in proposing improvements.
On the other hand, it is the responsibility of the reviewer to ensure that the quality of the CL does not reduce the overall quality of the code base over time. This can be difficult, because often degradation occurs due to a slight decrease in the quality of the code over time, especially if the team is under severe pressure from the deadlines and feels that it has the right to increase technical debt.
In addition, the reviewer is responsible for the code being reviewed. He wants to make sure that the code base remains consistent, supported, and matches everything else that is mentioned in the “What to check in the code” section .
Thus, we get the following rule as a standard for code review:
Typically, reviewers should endorse the CL as soon as it reaches a state where it definitely improves the overall quality of the system code, even if the CL is not perfect.
This is the main code review among all principles.
Of course, he has limitations. For example, if CL adds a function that the reviewer does not want to see in the system, then the reviewer can certainly refuse to commit, even if the code is of good quality.
The key point here is that there is no “perfect” code - there is only better code. The reviewer should not require the author to polish every tiny fragment. Rather, the reviewer must balance the need for further progress over the importance of the proposed changes. Instead of striving for the ideal, the reviewer should strive for continuous improvement . A commit, which generally improves the maintainability, readability, and comprehensibility of the system, cannot be delayed for days or weeks because it is not "perfect."
Reviewers can always leave any comments on improving the code, but not very important changes should be marked, for example, with the Nit prefix : so that the author knows that this is just a point of view that he can ignore.
Note. Nothing in this document justifies CLs that definitely degrade the overall quality of the system code. This is only possible in an emergency .
A code review can also be important for teaching developers something new about the language, structure, or general principles of software design. It is always nice to leave comments that help the developer learn something new. Sharing knowledge contributes to improving system code over time. Just keep in mind that if you leave a purely educational comment that is not critical to meeting the standards described here, add the Nit prefix to it : or otherwise indicate that the author is not required to allow it.
In any conflict, the first step should always be the desire of the developer and reviewer to come to a consensus based on the contents of this document and other documents in the CL Author’s Guide and this Reviewer Guide .
When it’s especially difficult to reach consensus, a personal meeting or video conference between the reviewer and the author can help (if you do, be sure to write down the results of the discussion in a comment to the commit for future readers).
If this does not solve the situation, then the most common way is escalation. Often it consists in a broader discussion with the team, attracting team lead, contacting the maintainer or development manager. Do not let the commit linger due to the fact that the author and reviewer cannot come to an agreement.
Note. When considering each of these items, be sure to consider the Code Review Standard .
The most important thing is to consider the overall project (design) in the code review. Do interactions between different parts of the code make sense? Does this change apply to your codebase or library? Does CL integrate well with the rest of the system? Is it time to add this functionality now?
Does this CL do what the developer intended? Is it good for users of this code? By “users” we mean both end users (if they are affected by the change) and developers (who will have to “use” this code in the future).
Basically, we expect that even before committing, developers will test their code well enough for it to work correctly. But as a reviewer, you still need to think about extreme cases, look for concurrency problems, try to think as a user, and even watch the code read that there are no obvious errors.
If you want, you can check the performance. This is most important if the code affects users, such as changing the UI . It's hard to understand how some changes will affect users when you just read the code. For such changes, you can ask the developer to provide a demo if it is too difficult for you to delve into the code and try it yourself.
Another point when it is especially important to think about functionality during a code review is if some kind of parallel programming occurs in the CL, which theoretically can cause deadlocks or race conditions. Such problems are very difficult to detect by simply running the code; usually it is necessary that someone (both the developer and the reviewer) carefully think over them and make sure that no problems are introduced (note that this is also a good reason not to use parallelism models where race conditions or deadlocks are possible - this can be done by code very difficult to understand or code review).
Is CL more complicated than it should be? Check it at every level: separate lines, functions, classes. “Excessive complexity” usually means the inability to quickly understand while reading . It may also mean that developers are more likely to introduce errors when trying to call or modify this code .
A special type of complexity is over-engineering, when developers have made the code more universal than it should be, or added functionality that the system does not currently need. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve a problem that should definitely be solved now, rather than a problem that may need to be resolved in the future. A future problem should be solved when it appears, and you can see its actual form and requirements in the physical Universe.
Request unit, integration, or end-to-end tests that are relevant to the change. In general, tests should be added to the same CL as the production code if the CL does not handle the emergency .
Make sure the tests are correct, reasonable, and helpful. Tests do not test themselves, and we rarely write tests for our tests - a person must make sure that the tests are valid.
Do tests really fail on broken code? If the code changes, will there be false positives? Does each test make simple and useful statements? Are tests correctly divided between different test methods?
Remember that tests are code that also has to be supported. Do not allow complexity in them just because it is not part of the main binary file.
Has the developer chosen good names everywhere? A good name is long enough to fully convey what the element is or what it does without being so long that it becomes difficult to read.
Did the developer write clear comments in plain language? Are all comments necessary? Comments are usually useful when they explain why some code exists, and should not explain what this code does. If the code is not clear enough to explain itself, then it should be simplified. There are some exceptions (for example, comments explaining the actions of the code are often very useful for regular expressions and complex algorithms), but mostly comments are intended for information that the code itself cannot contain, for example, substantiating a decision.
It may also be useful to look at the comments in the previous code. Perhaps there is a TODO, which can now be deleted, or a comment that does not recommend introducing this change, etc.
Note that comments differ from the documentation for classes, modules, or functions, which describes the task of the code, how it should be used, and how it behaves.
We have Google style guides for all major languages, and even most of the minor ones. Make sure the CL is not in conflict with the relevant style guides.
If you want to improve some elements that are not in the style guide, add a note to the comment ( Nit:) . The developer will know that this is your personal remark, which is not binding. Do not block sending commits based solely on your personal preferences.
The author should not combine significant style changes with other changes. This makes it difficult to see changes in the CL, complicates merges, code rollbacks, and causes other problems. For example, if the author wants to reformat the entire file, ask for a style change in one CL, and then send the CL with functional changes.
If the CL output changes something in the assembly, testing, interaction procedures, or code release, check for updates to the relevant documentation, including README files, g3doc pages, and all generated reference documents. If the CL removes code or makes it obsolete, consider whether to also remove the documentation. If no documentation is available, request its creation.
View each line in the code. Although any data files, generated code or large data structures can be briefly reviewed, but carefully read every class, function or code block written by a person, never assume by default that everything is in order. Obviously, some code deserves a more thorough study than another - you decide for yourself, but you should at least be sure that you understand the operation of the whole code.
If it is too difficult for you to read the code and this slows down the review, you should inform the developer and wait until he brings clarity before continuing. At Google, we hire great software engineers, and you are one of them. If you cannot understand the code, it is very likely that other developers will not be able to. In this way, you also help future developers understand this code when asking the developer for clarity.
If the code is understandable, but you don’t feel qualified enough to evaluate a fragment, make sure that there is a reviewer in the CL who is qualified, especially for complex problems such as security, concurrency, accessibility, internationalization, etc.
It is often useful to look at CL in a broad context. Typically, a code review tool only shows a few lines near the place of change. Sometimes you need to look at the entire file to make sure that the change really makes sense. For example, you see the addition of only four lines, but if you look at the entire file, then these four lines are in the 50-line method, which now really has to be broken into smaller ones.
It is also useful to think about CL in the context of the system as a whole. Does it improve the quality of the system code or makes it more complex, less tested, etc.? Do not accept commits that degrade system code. Most systems are complicated by the sum of many small changes, so it’s important to prevent even minor difficulties there.
If you see something good in the commit, let the developer know, especially when he has solved the problem described in one of your comments in the best possible way. Code reviews often simply focus on bugs, but they should also encourage and value good practice. From the point of view of mentoring, it is sometimes even more important to tell the developer what exactly he did right, and not where he made a mistake.
When executing a code review, make sure that:
During the review, be sure to look at each line of code, look at the context , make sure that you are improving the quality of the code and praise the developers for the good that they managed to do.
Now that you know what to check in code , what is the most efficient way to conduct code reviews on multiple files?
Look at the CL description and what it does in general. Does this change make sense at all? If it should not have been written initially, please immediately respond with an explanation of why it is not needed. When you reject such a change, it’s also nice to offer the developer what to do instead.
For example, you might say, “Looks like you did a good job, thanks!” However, we are actually going to remove the FooWidget system, so we don’t want to make any new changes right now. How about you refactoring our new BarWidget class instead? ”
Please note that the reviewer not only rejected the current CL and provided an alternative suggestion, but also did it politely . Such courtesy is important because we want to show that we respect each other as developers, even when we disagree with each other.
If you get quite a few unwanted CLs, you should review the development process on your team or published rules for external contributors to improve communication when writing the CL. It's better to tell people “no” before they do a ton of work that will have to be thrown away or rewritten a lot.
Find the file or files that represent the "main" part of this CL. Often there is one file with the most logical changes, and this is the main part of the CL. First look at these main parts. This helps to understand the context of smaller parts of the CL, and generally speeds up code review execution. If the CL is too large, ask the developer what to see first, or ask him to split the CL into parts .
If you see serious problems in the main part of the CL, you should immediately send these comments, even if you do not have time to view the rest of the CL right now. In fact, looking at the rest of the code can be a waste of time, because if the problems are significant enough, many other pieces of code in question will disappear and will not matter.
There are two main reasons why it is so important to send these main comments immediately:
After making sure that there are no serious problems with the design of the CL as a whole, try to figure out the logical sequence for viewing files and make sure that nothing is missed. Usually, when you look at the main files, the easiest way is to simply go through the rest in the order in which the code review tool presents them. It is also sometimes useful to read the tests first, and then the main code, because then you will have an idea of ​​what the meaning of the change is.
At Google, we optimize the speed of collaboration , not the speed of individual developers. The speed of individual work is important, it just is not so priority in comparison with the speed of team work.
When you slowly look at the code, several things happen:
If you are not in the middle of a focused task, you should make a review code shortly after it arrives.
One business day is the maximum time for an answer (that is, this is the first thing the next morning).
Following these guidelines means that a typical CL should receive several rounds of review (if necessary) within one day.
There is one moment when the priority of personal speed exceeds team speed. If you're in the middle of a focused task, such as writing code, don't be distracted by code review. Studies have shown that it can take a long time for a developer to return to a smooth development flow after distraction. Thus, the distraction from coding actually costs the team more than asking another developer to wait a bit for the code review.
Instead, wait for a breakpoint in your work. For example, after completing the current task, after lunch, returning from a meeting, returning from an office kitchenette, etc.
When we talk about code review speed, we are interested in the response time, and not how much time is required for the entire process to the end. Ideally, the whole process should also be fast, but it is even more important that individual answers quickly arrive than the whole process .
Even if the entire code review process takes a lot of time, the availability of quick responses from the reviewer throughout the process greatly simplifies the life of developers who may be annoyed by "slow" answers.
If you are too busy for a full review immediately after receiving the request, you can still quickly respond with a message about the deadlines or offer the review to other reviewers who can respond faster or provide some initial general comments (note: none of this means that you should interrupt the coding even to send such a response - send a response during a reasonable breakpoint in your work).
It is important that reviewers spend enough time reviewing and be sure that their “LGTM” exactly means “this code complies with our standards .” However, the individual answers should ideally be quick anyway.
When working between different time zones, try to answer the author while he is still in the office. If he has already gone home, then be sure to try to send an answer before he returns to the office the next day.
For speed reasons, there are certain situations in which the reviewer must give LGTM / approval, even in the case of unanswered comments on the CL. This is done if:
The reviewer should indicate which of these options he means if this is not clear.
Reserved LGTMs are especially useful when the developer and reviewer are in different time zones, otherwise the developer will wait all day to get “LGTM Approved.”
If someone sends you a review code so large that you are not sure when you can view it, then the typical answer would be to ask the developer to split the CL into several smaller CLs . This is usually possible and very useful for reviewers, even if it requires additional work from the developer.
If CL cannot be broken down into smaller CLs and you don’t have time to quickly look through all this, at least write some comments on the overall CL design and send them back to the developer for improvement. One of your goals as a reviewer should always be to “unlock” the developer or to allow him to quickly take any further action without sacrificing the quality of the code.
If you follow these recommendations and strictly approach the code review, you will find that the whole process accelerates and accelerates over time. The developers will find out what is required for high-quality code and send you CLs that are great from the very beginning, requiring less and less time to view. Reviewers learn to respond quickly and not add unnecessary delays to the review process. But do not compromise code review standards or quality for an imaginary speed improvement - in fact, you will not achieve overall acceleration in the long run.
There are also emergency situations when CLs have to go through the entire code review process very quickly, and where quality principles will have to be softened. But please read the description of which situations qualify as emergency and which do not.
In general, it is important to be polite and respectful, and also very clear and helpful to the developer whose code you are viewing. One way to do this is to make sure that you always make comments about the code and never about the developer . You do not always have to follow this practice, but you must use it when you say something unpleasant or controversial. For example:
Bad: “Why did you use streams here, if it is obvious that parallelism does not give any benefit?”
Good: “The concurrency model here adds complexity to the system, and I don’t see any actual performance benefit. Since there is no performance benefit, it is best for this code to be single-threaded rather than using multiple threads. ”
In the “good” example above, you can see that it helps the developer understand why you are making your comment. You do not always need to include this information in your comments, but sometimes it’s appropriate to give a little more explanation about your logic, the best practices that you follow, or how your suggestion improves the quality of the code.
In general, making corrections is the task of the developer, not the reviewer. You are not required to develop a detailed draft solution or write code for the developer.
However, this does not mean that the reviewer cannot help here. In general, an appropriate balance should be struck between identifying problems and providing direct assistance. Pointing out problems and giving the developer the opportunity to make a decision often helps the developer to learn and facilitates code review execution. It can also lead to a better solution, because the developer is closer to the code than the reviewer.
However, direct instructions, suggestions, or even code are sometimes more useful. The main purpose of a code review is to get the best possible CL. The secondary goal is to improve the skills of developers so that the review takes less and less time for them.
If you ask the developer to explain an incomprehensible piece of code, usually this will lead to him rewriting it more clearly . Sometimes adding a comment is also a good answer, if it is not just an explanation of too complex code.
Explanations written only in the review tool will not help future readers. They are acceptable only in some cases, for example, when you look at an area you are not very familiar with, and the developer explains what ordinary code readers should already know.
Sometimes a developer argues in a code review process. Either he does not agree with your proposal, or complains that you are too strict in general.
When the developer does not agree with your proposal, first take the time to consider his position. Often it is closer to your code and therefore it may really have a better idea of ​​some aspects. Does it make sense in his argument? Does it make sense in terms of code quality? If so, then admit that he is right, and the question will disappear.
However, developers are not always right. In this case, the reviewer must further explain why he believes that his proposal is correct. A good explanation demonstrates both an understanding of the developer's response and additional information on why a change is required.
In particular, when the reviewer believes that his proposal will improve the quality of the code, he should insist on it if he believes that the resulting quality improvement justifies the additional work. Improving the quality of the code, as a rule, occurs in small steps.
Sometimes it takes several rounds of explanation before it is actually accepted. Just make sure that you always remain polite , and let the developer know that you hear it, just do not agree.
Reviewers sometimes feel that a developer is upset if a reviewer insists on improvement. Sometimes developers are really upset, but usually not for long, and later they will be very grateful that you helped them improve the quality of their code. Usually, if you are polite in your comments, the developers are not really upset at all, and the concern is only in the head of the reviewer. Usually the style of writing comments is more frustrating than the persistence of the reviewer regarding the quality of the code.
A typical source of controversy is that developers (for obvious reasons) want to get the job done. They do not want to go through another round of reviews for this CL. Therefore, they say that they will remove something in the later CL, and now you must make LGTM for this CL. Some developers do this very well, and immediately write another CL that will fix the problem. However, experience has shown that the more time elapses after the original CL, the less likely that this edit will occur. In fact, usually if the developer does not make the edit immediatelythen usually never does. Not because developers are irresponsible, but because they have a lot of work, and editing is lost or forgotten under the ramp of other work. Thus, it is usually best to insist on an immediate fix before the commit goes into the codebase and is “completed”. Allow pending edits is a typical way of degenerating code bases.
If the CL introduces a new complexity, then it must be fixed before sending, if this is not an emergency. If CL shows other problems that cannot be solved right now, the developer should register a bug in the tracker and assign it to himself so that he does not get lost. He can optionally write a TODO comment in the code regarding this error.
If you used to conduct a rather superficial code review, and switch to strict mode, some developers will start to complain very loudly. Increased code review speed usually resolves these complaints.
Sometimes it may take months for complaints to disappear, but in the end, developers tend to see the value of strict code reviews as they see how great the resulting code is. Sometimes the loudest protesters even become your strongest supporters when something happens that makes them really see the value of rigorous reviews.
If you are doing all of the above but still encounter conflicts, refer to the Code Review Standard section for recommendations and principles that can help resolve the conflict.
This guide provides recommendations for optimizing code reviews based on years of experience. Together, they make up one document, divided into many sections. It is not necessary to read all of them, but often it is better for yourself and the team to study the entire manual.
- Code Review Standard
- What to check in code
- Change List Navigation (CL)
- Code review speed
- How to write comments
- How to overcome resistance
See also the CL Author’s Guide for detailed advice on developers whose commits are reviewed.
Code Review Standard
The main purpose of a code review is to guarantee continuous improvement of the Google codebase. All tools and processes are dedicated to this goal.
A number of compromises are needed here.
First, developers should be able to successfully solve their problems . If you never send a code, then the code base will never improve. In addition, if the reviewer greatly complicates any work, then in the future developers are not interested in proposing improvements.
On the other hand, it is the responsibility of the reviewer to ensure that the quality of the CL does not reduce the overall quality of the code base over time. This can be difficult, because often degradation occurs due to a slight decrease in the quality of the code over time, especially if the team is under severe pressure from the deadlines and feels that it has the right to increase technical debt.
In addition, the reviewer is responsible for the code being reviewed. He wants to make sure that the code base remains consistent, supported, and matches everything else that is mentioned in the “What to check in the code” section .
Thus, we get the following rule as a standard for code review:
Typically, reviewers should endorse the CL as soon as it reaches a state where it definitely improves the overall quality of the system code, even if the CL is not perfect.
This is the main code review among all principles.
Of course, he has limitations. For example, if CL adds a function that the reviewer does not want to see in the system, then the reviewer can certainly refuse to commit, even if the code is of good quality.
The key point here is that there is no “perfect” code - there is only better code. The reviewer should not require the author to polish every tiny fragment. Rather, the reviewer must balance the need for further progress over the importance of the proposed changes. Instead of striving for the ideal, the reviewer should strive for continuous improvement . A commit, which generally improves the maintainability, readability, and comprehensibility of the system, cannot be delayed for days or weeks because it is not "perfect."
Reviewers can always leave any comments on improving the code, but not very important changes should be marked, for example, with the Nit prefix : so that the author knows that this is just a point of view that he can ignore.
Note. Nothing in this document justifies CLs that definitely degrade the overall quality of the system code. This is only possible in an emergency .
Mentoring
A code review can also be important for teaching developers something new about the language, structure, or general principles of software design. It is always nice to leave comments that help the developer learn something new. Sharing knowledge contributes to improving system code over time. Just keep in mind that if you leave a purely educational comment that is not critical to meeting the standards described here, add the Nit prefix to it : or otherwise indicate that the author is not required to allow it.
Principles
- Technical facts and data outweigh opinions and personal preferences.
- In matters of style, absolute authority is the guide to style . Any purely stylistic detail (space, etc.) that is not included in the style guide is a matter of personal preference. Style should match what it is. If there is no previous style, accept the author.
- Aspects of software design are almost never a problem of pure style or personal preference. They are based on fundamental principles and should be determined on these principles, and not just on personal opinion. Sometimes there are several valid options. If the author can demonstrate (either using data or based on solid engineering principles) that certain approaches are equally effective, the reviewer should accept the author’s preference. Otherwise, the choice is dictated by standard development principles.
- If no other rule is applicable, then the reviewer may ask the author to observe uniformity with the current code base, if this does not worsen the general condition of the system.
Conflict resolution
In any conflict, the first step should always be the desire of the developer and reviewer to come to a consensus based on the contents of this document and other documents in the CL Author’s Guide and this Reviewer Guide .
When it’s especially difficult to reach consensus, a personal meeting or video conference between the reviewer and the author can help (if you do, be sure to write down the results of the discussion in a comment to the commit for future readers).
If this does not solve the situation, then the most common way is escalation. Often it consists in a broader discussion with the team, attracting team lead, contacting the maintainer or development manager. Do not let the commit linger due to the fact that the author and reviewer cannot come to an agreement.
What to check in code
Note. When considering each of these items, be sure to consider the Code Review Standard .
Design
The most important thing is to consider the overall project (design) in the code review. Do interactions between different parts of the code make sense? Does this change apply to your codebase or library? Does CL integrate well with the rest of the system? Is it time to add this functionality now?
Functionality
Does this CL do what the developer intended? Is it good for users of this code? By “users” we mean both end users (if they are affected by the change) and developers (who will have to “use” this code in the future).
Basically, we expect that even before committing, developers will test their code well enough for it to work correctly. But as a reviewer, you still need to think about extreme cases, look for concurrency problems, try to think as a user, and even watch the code read that there are no obvious errors.
If you want, you can check the performance. This is most important if the code affects users, such as changing the UI . It's hard to understand how some changes will affect users when you just read the code. For such changes, you can ask the developer to provide a demo if it is too difficult for you to delve into the code and try it yourself.
Another point when it is especially important to think about functionality during a code review is if some kind of parallel programming occurs in the CL, which theoretically can cause deadlocks or race conditions. Such problems are very difficult to detect by simply running the code; usually it is necessary that someone (both the developer and the reviewer) carefully think over them and make sure that no problems are introduced (note that this is also a good reason not to use parallelism models where race conditions or deadlocks are possible - this can be done by code very difficult to understand or code review).
Complexity
Is CL more complicated than it should be? Check it at every level: separate lines, functions, classes. “Excessive complexity” usually means the inability to quickly understand while reading . It may also mean that developers are more likely to introduce errors when trying to call or modify this code .
A special type of complexity is over-engineering, when developers have made the code more universal than it should be, or added functionality that the system does not currently need. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve a problem that should definitely be solved now, rather than a problem that may need to be resolved in the future. A future problem should be solved when it appears, and you can see its actual form and requirements in the physical Universe.
Tests
Request unit, integration, or end-to-end tests that are relevant to the change. In general, tests should be added to the same CL as the production code if the CL does not handle the emergency .
Make sure the tests are correct, reasonable, and helpful. Tests do not test themselves, and we rarely write tests for our tests - a person must make sure that the tests are valid.
Do tests really fail on broken code? If the code changes, will there be false positives? Does each test make simple and useful statements? Are tests correctly divided between different test methods?
Remember that tests are code that also has to be supported. Do not allow complexity in them just because it is not part of the main binary file.
Naming
Has the developer chosen good names everywhere? A good name is long enough to fully convey what the element is or what it does without being so long that it becomes difficult to read.
Comments
Did the developer write clear comments in plain language? Are all comments necessary? Comments are usually useful when they explain why some code exists, and should not explain what this code does. If the code is not clear enough to explain itself, then it should be simplified. There are some exceptions (for example, comments explaining the actions of the code are often very useful for regular expressions and complex algorithms), but mostly comments are intended for information that the code itself cannot contain, for example, substantiating a decision.
It may also be useful to look at the comments in the previous code. Perhaps there is a TODO, which can now be deleted, or a comment that does not recommend introducing this change, etc.
Note that comments differ from the documentation for classes, modules, or functions, which describes the task of the code, how it should be used, and how it behaves.
Style
We have Google style guides for all major languages, and even most of the minor ones. Make sure the CL is not in conflict with the relevant style guides.
If you want to improve some elements that are not in the style guide, add a note to the comment ( Nit:) . The developer will know that this is your personal remark, which is not binding. Do not block sending commits based solely on your personal preferences.
The author should not combine significant style changes with other changes. This makes it difficult to see changes in the CL, complicates merges, code rollbacks, and causes other problems. For example, if the author wants to reformat the entire file, ask for a style change in one CL, and then send the CL with functional changes.
Documentation
If the CL output changes something in the assembly, testing, interaction procedures, or code release, check for updates to the relevant documentation, including README files, g3doc pages, and all generated reference documents. If the CL removes code or makes it obsolete, consider whether to also remove the documentation. If no documentation is available, request its creation.
Each line
View each line in the code. Although any data files, generated code or large data structures can be briefly reviewed, but carefully read every class, function or code block written by a person, never assume by default that everything is in order. Obviously, some code deserves a more thorough study than another - you decide for yourself, but you should at least be sure that you understand the operation of the whole code.
If it is too difficult for you to read the code and this slows down the review, you should inform the developer and wait until he brings clarity before continuing. At Google, we hire great software engineers, and you are one of them. If you cannot understand the code, it is very likely that other developers will not be able to. In this way, you also help future developers understand this code when asking the developer for clarity.
If the code is understandable, but you don’t feel qualified enough to evaluate a fragment, make sure that there is a reviewer in the CL who is qualified, especially for complex problems such as security, concurrency, accessibility, internationalization, etc.
Context
It is often useful to look at CL in a broad context. Typically, a code review tool only shows a few lines near the place of change. Sometimes you need to look at the entire file to make sure that the change really makes sense. For example, you see the addition of only four lines, but if you look at the entire file, then these four lines are in the 50-line method, which now really has to be broken into smaller ones.
It is also useful to think about CL in the context of the system as a whole. Does it improve the quality of the system code or makes it more complex, less tested, etc.? Do not accept commits that degrade system code. Most systems are complicated by the sum of many small changes, so it’s important to prevent even minor difficulties there.
Good
If you see something good in the commit, let the developer know, especially when he has solved the problem described in one of your comments in the best possible way. Code reviews often simply focus on bugs, but they should also encourage and value good practice. From the point of view of mentoring, it is sometimes even more important to tell the developer what exactly he did right, and not where he made a mistake.
Summary
When executing a code review, make sure that:
- The code is well designed.
- Functionality is good for code users.
- Any changes to the UI are reasonable and look good.
- Any concurrent programming is safe.
- The code is no more complicated than it should be.
- The developer does not implement what may be needed in the future with unknown prospects.
- The code is lined with appropriate unit tests.
- The tests are well designed.
- The developer everywhere used clear names.
- Comments are understandable and useful, and basically answer the question why? but not what?
- The code is properly documented (usually in g3doc).
- The code complies with our style guides.
During the review, be sure to look at each line of code, look at the context , make sure that you are improving the quality of the code and praise the developers for the good that they managed to do.
CL navigation in code review
Summary
Now that you know what to check in code , what is the most efficient way to conduct code reviews on multiple files?
- Does this change make sense? Does he have a good description ?
- First look at the most important part. Is it well designed?
- Look at the rest of the CL in the appropriate sequence.
Step one: take a look around the entire commit
Look at the CL description and what it does in general. Does this change make sense at all? If it should not have been written initially, please immediately respond with an explanation of why it is not needed. When you reject such a change, it’s also nice to offer the developer what to do instead.
For example, you might say, “Looks like you did a good job, thanks!” However, we are actually going to remove the FooWidget system, so we don’t want to make any new changes right now. How about you refactoring our new BarWidget class instead? ”
Please note that the reviewer not only rejected the current CL and provided an alternative suggestion, but also did it politely . Such courtesy is important because we want to show that we respect each other as developers, even when we disagree with each other.
If you get quite a few unwanted CLs, you should review the development process on your team or published rules for external contributors to improve communication when writing the CL. It's better to tell people “no” before they do a ton of work that will have to be thrown away or rewritten a lot.
Step Two: Learn the Basic Parts of CL
Find the file or files that represent the "main" part of this CL. Often there is one file with the most logical changes, and this is the main part of the CL. First look at these main parts. This helps to understand the context of smaller parts of the CL, and generally speeds up code review execution. If the CL is too large, ask the developer what to see first, or ask him to split the CL into parts .
If you see serious problems in the main part of the CL, you should immediately send these comments, even if you do not have time to view the rest of the CL right now. In fact, looking at the rest of the code can be a waste of time, because if the problems are significant enough, many other pieces of code in question will disappear and will not matter.
There are two main reasons why it is so important to send these main comments immediately:
- Developers often send CL, and then immediately start a new job based on it, while waiting for the result of a code review. If the CL has serious problems, they will have to rework the next CL. It is advisable to identify errors as early as possible before they do a lot of extra work on top of the problematic design.
- Major changes take longer than minor edits. Almost all developers have deadlines. In order to keep within them and not reduce the quality of the code, any major refactoring should begin as soon as possible.
Step Three: Scroll through the rest of the CL in the appropriate sequence.
After making sure that there are no serious problems with the design of the CL as a whole, try to figure out the logical sequence for viewing files and make sure that nothing is missed. Usually, when you look at the main files, the easiest way is to simply go through the rest in the order in which the code review tool presents them. It is also sometimes useful to read the tests first, and then the main code, because then you will have an idea of ​​what the meaning of the change is.
Code review speed
Why should a code review be done quickly?
At Google, we optimize the speed of collaboration , not the speed of individual developers. The speed of individual work is important, it just is not so priority in comparison with the speed of team work.
When you slowly look at the code, several things happen:
- The speed of the team as a whole decreases. Yes, if a person does not respond quickly to a code review, he does another job. However, for the rest of the team, new features and bug fixes are delayed by days, weeks, or months, as each CL waits for a code review and a repeated code review.
- Developers begin to protest against the code review process. If the reviewer responds only after a few days, but each time asks for serious changes, this can be unpleasant and difficult for developers. This is often expressed in complaints about how “strict” a reviewer is. If the reviewer requests the same significant changes (changes that really improve the performance of the code), but reacts quickly every time, complaints usually disappear. Most complaints about the code review process are actually resolved by speeding up the process.
- The quality of the code may suffer. Slowing down the code review increases the risk that developers will not send as good CLs as they could be. Slow reviews also hinder code cleanup, refactoring, and further improvements to existing CLs.
How to quickly execute a code review?
If you are not in the middle of a focused task, you should make a review code shortly after it arrives.
One business day is the maximum time for an answer (that is, this is the first thing the next morning).
Following these guidelines means that a typical CL should receive several rounds of review (if necessary) within one day.
Speed ​​and distraction
There is one moment when the priority of personal speed exceeds team speed. If you're in the middle of a focused task, such as writing code, don't be distracted by code review. Studies have shown that it can take a long time for a developer to return to a smooth development flow after distraction. Thus, the distraction from coding actually costs the team more than asking another developer to wait a bit for the code review.
Instead, wait for a breakpoint in your work. For example, after completing the current task, after lunch, returning from a meeting, returning from an office kitchenette, etc.
Quick replies
When we talk about code review speed, we are interested in the response time, and not how much time is required for the entire process to the end. Ideally, the whole process should also be fast, but it is even more important that individual answers quickly arrive than the whole process .
Even if the entire code review process takes a lot of time, the availability of quick responses from the reviewer throughout the process greatly simplifies the life of developers who may be annoyed by "slow" answers.
If you are too busy for a full review immediately after receiving the request, you can still quickly respond with a message about the deadlines or offer the review to other reviewers who can respond faster or provide some initial general comments (note: none of this means that you should interrupt the coding even to send such a response - send a response during a reasonable breakpoint in your work).
It is important that reviewers spend enough time reviewing and be sure that their “LGTM” exactly means “this code complies with our standards .” However, the individual answers should ideally be quick anyway.
Code review between time zones
When working between different time zones, try to answer the author while he is still in the office. If he has already gone home, then be sure to try to send an answer before he returns to the office the next day.
Reserved LGTM
For speed reasons, there are certain situations in which the reviewer must give LGTM / approval, even in the case of unanswered comments on the CL. This is done if:
- The reviewer is confident that the developer will properly consider all remaining comments.
- Other changes are minor and optional .
The reviewer should indicate which of these options he means if this is not clear.
Reserved LGTMs are especially useful when the developer and reviewer are in different time zones, otherwise the developer will wait all day to get “LGTM Approved.”
Large CL
If someone sends you a review code so large that you are not sure when you can view it, then the typical answer would be to ask the developer to split the CL into several smaller CLs . This is usually possible and very useful for reviewers, even if it requires additional work from the developer.
If CL cannot be broken down into smaller CLs and you don’t have time to quickly look through all this, at least write some comments on the overall CL design and send them back to the developer for improvement. One of your goals as a reviewer should always be to “unlock” the developer or to allow him to quickly take any further action without sacrificing the quality of the code.
Code review improvements over time
If you follow these recommendations and strictly approach the code review, you will find that the whole process accelerates and accelerates over time. The developers will find out what is required for high-quality code and send you CLs that are great from the very beginning, requiring less and less time to view. Reviewers learn to respond quickly and not add unnecessary delays to the review process. But do not compromise code review standards or quality for an imaginary speed improvement - in fact, you will not achieve overall acceleration in the long run.
Emergencies
There are also emergency situations when CLs have to go through the entire code review process very quickly, and where quality principles will have to be softened. But please read the description of which situations qualify as emergency and which do not.
How to write comments in code review
Summary
- Be polite.
- Explain your reasoning.
- Avoid explicit orders by simply pointing out problems and letting the developer decide.
- Encourage developers to simplify code or add comments instead of simple explanations for complexity.
Politeness
In general, it is important to be polite and respectful, and also very clear and helpful to the developer whose code you are viewing. One way to do this is to make sure that you always make comments about the code and never about the developer . You do not always have to follow this practice, but you must use it when you say something unpleasant or controversial. For example:
Bad: “Why did you use streams here, if it is obvious that parallelism does not give any benefit?”
Good: “The concurrency model here adds complexity to the system, and I don’t see any actual performance benefit. Since there is no performance benefit, it is best for this code to be single-threaded rather than using multiple threads. ”
Explain the reasons
In the “good” example above, you can see that it helps the developer understand why you are making your comment. You do not always need to include this information in your comments, but sometimes it’s appropriate to give a little more explanation about your logic, the best practices that you follow, or how your suggestion improves the quality of the code.
Directions
In general, making corrections is the task of the developer, not the reviewer. You are not required to develop a detailed draft solution or write code for the developer.
However, this does not mean that the reviewer cannot help here. In general, an appropriate balance should be struck between identifying problems and providing direct assistance. Pointing out problems and giving the developer the opportunity to make a decision often helps the developer to learn and facilitates code review execution. It can also lead to a better solution, because the developer is closer to the code than the reviewer.
However, direct instructions, suggestions, or even code are sometimes more useful. The main purpose of a code review is to get the best possible CL. The secondary goal is to improve the skills of developers so that the review takes less and less time for them.
Accept the explanation
If you ask the developer to explain an incomprehensible piece of code, usually this will lead to him rewriting it more clearly . Sometimes adding a comment is also a good answer, if it is not just an explanation of too complex code.
Explanations written only in the review tool will not help future readers. They are acceptable only in some cases, for example, when you look at an area you are not very familiar with, and the developer explains what ordinary code readers should already know.
How to overcome resistance in the code review process
Sometimes a developer argues in a code review process. Either he does not agree with your proposal, or complains that you are too strict in general.
Who is right?
When the developer does not agree with your proposal, first take the time to consider his position. Often it is closer to your code and therefore it may really have a better idea of ​​some aspects. Does it make sense in his argument? Does it make sense in terms of code quality? If so, then admit that he is right, and the question will disappear.
However, developers are not always right. In this case, the reviewer must further explain why he believes that his proposal is correct. A good explanation demonstrates both an understanding of the developer's response and additional information on why a change is required.
In particular, when the reviewer believes that his proposal will improve the quality of the code, he should insist on it if he believes that the resulting quality improvement justifies the additional work. Improving the quality of the code, as a rule, occurs in small steps.
Sometimes it takes several rounds of explanation before it is actually accepted. Just make sure that you always remain polite , and let the developer know that you hear it, just do not agree.
About the resentment of developers
Reviewers sometimes feel that a developer is upset if a reviewer insists on improvement. Sometimes developers are really upset, but usually not for long, and later they will be very grateful that you helped them improve the quality of their code. Usually, if you are polite in your comments, the developers are not really upset at all, and the concern is only in the head of the reviewer. Usually the style of writing comments is more frustrating than the persistence of the reviewer regarding the quality of the code.
Pending edits
A typical source of controversy is that developers (for obvious reasons) want to get the job done. They do not want to go through another round of reviews for this CL. Therefore, they say that they will remove something in the later CL, and now you must make LGTM for this CL. Some developers do this very well, and immediately write another CL that will fix the problem. However, experience has shown that the more time elapses after the original CL, the less likely that this edit will occur. In fact, usually if the developer does not make the edit immediatelythen usually never does. Not because developers are irresponsible, but because they have a lot of work, and editing is lost or forgotten under the ramp of other work. Thus, it is usually best to insist on an immediate fix before the commit goes into the codebase and is “completed”. Allow pending edits is a typical way of degenerating code bases.
If the CL introduces a new complexity, then it must be fixed before sending, if this is not an emergency. If CL shows other problems that cannot be solved right now, the developer should register a bug in the tracker and assign it to himself so that he does not get lost. He can optionally write a TODO comment in the code regarding this error.
General complaints of severity
If you used to conduct a rather superficial code review, and switch to strict mode, some developers will start to complain very loudly. Increased code review speed usually resolves these complaints.
Sometimes it may take months for complaints to disappear, but in the end, developers tend to see the value of strict code reviews as they see how great the resulting code is. Sometimes the loudest protesters even become your strongest supporters when something happens that makes them really see the value of rigorous reviews.
Conflict resolution
If you are doing all of the above but still encounter conflicts, refer to the Code Review Standard section for recommendations and principles that can help resolve the conflict.
All Articles