How to do a good Code Review

As technologists, you will find that there are many aspects of our work that seem “metaphysical”. Such as performance optimization, such as technical management, such as the Code Review mentioned in this article. It seems that everyone has some experience in these technical fields, but there are few companies, teams and individuals that can really do these things well, as far as I know. Take Code Review as an example, even some large domestic factories did not do this well. I often ask candidates who have worked in large factories for many years “how did your team do a good job in Code Review?” during the interview. . From the answers, we can find that few people have thought deeply about this, which also reflects that there are not many teams who can do this well.

Why is that? According to my current understanding, it is not enough to solve these complex problems only by relying on the experience of some discrete points. We also need to use some thinking frames and the standard operating procedures embodied based on them.

As one of the most advanced Internet companies, Google has accumulated a lot of excellent engineering practice experience. As for how to do a good job in Code Review, I also have my own methodology: Code Review Developer Guide. I spent more than two hours reading it carefully over the weekend and was very inspired. The following content is based on this Code Review guide (hereinafter referred to as the “guide”), combined with my own thinking, to sort out “how to do a good Code Review” in the way of Code Review workflow. If you have the time, it’s highly recommended that you read Google’s original article.

Zero. Core principles

The first part of The guide, The Standard of Code Review, explains The core principles of Code Review. Conclusion: As Reviewer (Reviewer below), the criterion for accepting this code change is not that it must be perfect, but that it will improve the health of the entire code base. How to understand this core principle? First of all, there is no “perfect code”. There is no one who defines perfect code. What we need is better code and continuous improvement of the code base. “Perfect code” is static thinking, “continuous improvement” is dynamic thinking. Reviewer should not reject code changes that would improve the overall system’s readability and maintainability on the grounds of “perfect code.”

The purpose of Code Review is to continuously iterate quickly over the long term and support business delivery with high efficiency and quality. If the pursuit of “perfection” leads to project delays, it is putting the cart before the horse. Of course this doesn’t mean lowering standards, basic entry barriers (like a unified style guide, Sonar quality domain, etc.) are strictly adhered to, and beyond that, if you can improve the codebase (or at least not corrupt it), you shouldn’t be looking for perfection.

This is the first principle of Code Review. However, after reading through this guide, there is another important principle that requires empathy, whether it is development with Reviewer or submission of changes. Because Code Review is a cooperative behavior of both parties, how to do it well efficiently and happily depends on both parties to think more about each other, and how to do it will be described in detail below.

Summary, core principles of Code Review:

• The rule of continuous Improvement: Acceptance of changes is based on whether the code submitted will improve (or at least not corrupt) the codebase. Strive for continuous improvement, not perfect code.
• Principle of empathy: Code Review is a collaboration involving Reviewer and Developer. To do this well, both parties should think for each other.

How does Developer submit changes

Many companies use GitLab as a tool for code versioning and collaboration. Developer initiated Code Review by submitting a Merge Request (MR) and assigning the MR to the Reviewer. So how should Developer submit this change to improve the efficiency of Code Review?

The CL Author’s Guide section of The guide covers two of The most important tips:

1. Write a description of the change
2. Commit small changes at a time

What to make of these two suggestions?

On “Writing a description of this change” : First understand why. A Reviewer is usually the Reviewer of a code base (or one of the modules) and is usually in a more senior development role, which means they are likely to be busy and time is at a premium. Of course, even if it is common development with Reviewer (s), out of respect for others’ time (with empathy, you also hope others will respect your time), as the Developer who initiated this Code Review, it is our responsibility to clearly describe the change: what was changed and why? In addition, there is a need to write a change description: future developers may need to understand why you made the change, and it is much more effective to trace back to the change you committed and see a clear description of the change than to read the code to understand why.

How do you write a description of the changes? Google suggests the following format:

Line 1: State in a full sentence what was changed, followed by a blank line

Content: Describe the content of the change in detail, providing supplementary information necessary to understand the change, such as what problem was solved, why the change was made, and what problems might be caused by the change. If possible, add additional background information, such as relevant design documents, bug fix task links, etc.

A good example:

Prettier is introduced to ensure code is formatted consistently.

In the previous project, there was no good code format constraint tool, and the code formats written by different people were very different. For example, some people liked a long line, some people liked a short line, some people wrote code without blank lines, and some people liked to add blank lines at random. Unify the code format specification of the project by introducing Prettier, currently the most popular automatic code formatting tool on the front end.

On “commit small changes at a time” : Understand why in the first place. There are some obvious benefits to submitting small changes at a time, such as the review process being faster and more comprehensive, bugs being less likely to be introduced, and code design problems being found sooner rather than having to write a lot and get rejected and rewritten. Also based on the principle of “empathy”, if a large number of changes were to be submitted at one time, the Reviewer would be a significant burden. He needs a big chunk of time to review your code in addition to his heavy workload. If he’s doing his job, he’s going to put a lot of effort into keeping your change from introducing bugs, going back and forth with you a lot, and it’s a long process that can significantly affect the progress of the project. Of course, it is also possible that he will review selectively, which can lead to code base corruption. In either case, this is the opposite of the ultimate goal of Code Review. So Developer is responsible for submitting small changes each time.

So what is a small change? The criterion of “small” should not only be the number of lines of code change, but should be measured by the amount of mental burden it would place on the Reviewer. Of course, a change of around 100 lines is usually recommended, and 200 lines may be acceptable, but if you submit 500 lines, or even 1,000 lines, it will definitely be a Reviewer’s burden. Before submitting a change, you should ask yourself, would it be easier for me to review the change if I were a Reviewer? If the answer is no, try to break the changes down into smaller MR tasks. This is also an application of the principle of empathy.

So how do you commit as few changes as possible? Google has some suggestions, such as submitting code for refactoring, bug fixes, feature additions, etc., separately, but the final measure of what constitutes refactoring and what constitutes feature additions is in the developer’s mind. Everyone may have their own tips and insights on how to break up, but the Developer submitting the change should always submit small changes as a guideline to make sure the Reviewer does not have a difficult time performing the review.

Of course, there may be some special situations that cannot be broken down into small changes. It is recommended to say hello with Reviewer in advance, so that they are fully prepared for how much time and effort they need to invest. You see, this is also an application of the principle of empathy.

Ii. How will Reviewer review changes

What should the Reviewer receiving a change review request do after Developer submits a change according to the above criteria? The guide provides operational suggestions in How to do a Code Review, which I summarize as follows:

1. Respond to Review requests as quickly as possible
2. Review code in multiple dimensions
3. Write a good Code Review

The following describes the three operation suggestions.

On “Respond to Review requests as quickly as possible” : Code Review is a part of r&d, like any other process node, to make a product or deliver a project better and faster. If Code Review is too slow, it will seriously affect the team’s r&d performance. A lot of developers who are reviewed will resent the review itself and feel that it is hindering them rather than helping them.

How quickly should you respond? Google recommends no more than a working day. Of course there may be some contradictions. We emphasize that the programmer’s flow should not be interrupted, because it takes extra time to get back to that productive state of work. If the Reviewer is in this position, please finish your own work first, or do a Code Review after the meeting or lunch break. I think the criterion for quick response is that we seek to maximize the overall efficiency of the team, not the efficiency of the Reviewer or the person submitting the change.

What needs to be emphasized here is that we have been talking about “quick response”, rather than completing Code Review quickly. Code Review itself should certainly be as fast as possible, but Review itself takes time. We cannot do it for the sake of fast, and the basis of fast is that the submitted Code must meet our standards. However, as a matter of empathy, we should respond as quickly as possible to reduce the waiting anxiety of developers committing changes. If Reviwer is really too busy for time, you can suggest other qualified Reviewer to respond to this Code Review quickly, or submit some overall suggestions for changes first.

We mentioned earlier that as a Developer you should submit as few changes as possible, but it’s possible that some people don’t follow this guideline. If the submitted change is significant, as a Reviewer, the Developer may be requested to change to multiple smaller Code Review requests. If it really isn’t possible to split, and you don’t have a lot of time to do a full review quickly, you should at least submit some feedback, such as comments on the overall design, so that the Developer who submitted the changes can take quick action to improve. The general rule is not to block Developer’s workflow as much as possible without sacrificing code health.

As long as you keep doing this, a good positive cycle will be formed. As time goes by, Code Review itself will take less time and become faster and faster.

With regard to “Review code in multiple dimensions” : As a Reviewer, how will code be reviewed and which dimensions of code quality should you focus on? There may be some differences between stacks, and this is where many Code Review experience sharing articles have been written. I won’t go into it here, because it’s going to be a lot of detail. Here are a few dimensions of Google’s advice:

• Overall design: The code should be well designed. A few more questions should be asked as a Reviewer: Does the code involved in the change really work? Should the change be placed in the project or in the dependency library? Do we need to add this functionality now? If there are problems with the overall design, feedback should be given to Developer as soon as possible to avoid him wasting too much time in the wrong direction.
• Functional impact: Make sure this change is friendly to users of the code (end users and other developers). Also ask yourself some questions: Will this change lead to a bad user experience (performance stability)? Is the UI not broken? Does it affect other parts of parallel development?
• Complexity: Code should not be more complex than it needs to be. If a piece of code is not well understood, it is probably too complex. Complex code is also more likely to introduce bugs in subsequent maintenance. Also, don’t over-engineer and encourage Devloper to solve problems that need to be solved today, not problems that might need to be solved tomorrow.
• Unit testing: Code should have reasonable unit testing. This should be common sense in all development. And the single test itself should have a good design, test code to avoid too complex.
• Naming: There should be good enough naming; the code is the document. Good naming should contain enough information and not require additional comments.
• Comments: Comments should explain why, not what the code is doing. If the code doesn’t explain itself, the code should be refactored more simply.
• Documentation: If the code changes, you should think about whether the relevant documentation should change as well, and you should keep the documentation up to date.
• Style guidelines: Code must follow common style guidelines to maintain style consistency.

Other than that, here are some suggestions:

• Review each line of code: In principle, each line of code should be reviewed carefully as a Reviewer. Of course, there are still some standards to grasp, such as some configuration files, data files, etc., may only need to glance at it. The core code might take more time than just looking at it once.
• Pay attention to the context of the change. Sometimes the changes are just a few lines of code. If you can’t find the problem just by looking at a few lines of code, you should look at the context in which you changed the code, which is likely to reveal the potential problem. Always remember the top rule, submitted code should not degrade the health of the code base.
• Don’t be stingy with compliments. If you see something good about the changes that Developer has made, such as improvements to the existing methods, or new techniques that you learned from them, you should praise them in the comments.

Finally, about “Write a good Code Review Review” : The reason is obvious, this is a key step in Code Review to form a closed loop and provide effective feedback to Developer. The guide also gives us a few tips on how to do it:

• To be friendly. Focus on the issue, not the person, to avoid unnecessary arguments. This is also an application of the principle of empathy, which makes it a little uncomfortable to be told off politely. Bad sentences: “You shouldn’t write like this, XXX.” Good sentences: “This place may have XXX problems in this way. Would it be better to change it to XXX?”
• Explain why. Explain why this is an important weapon in the doctrine of influence. While it’s not required that every comment come with an explanation, if you can explain why you made the comment and make it clear, your reviewers will be more receptive and likely to discuss it more calmly.
• Give guidance. This is a trade-off, sometimes just pointing out the problem, because developers have a better understanding of the context and context of the code being changed, and they may be able to come up with a better solution once they know the problem. Of course, sometimes, as a Reviewer, you may also put forward specific suggestions for improvement.
• Improvements fall into the code. Sometimes you don’t understand a piece of code and need to be explained by Developer, but it’s best not to stop there. You should have Developer refactor the code to make it easier to understand, or add necessary comments to the code. This makes it easier for subsequent maintainers to understand the code.

Iii. How to resolve dissenting opinions

This part is mainly about the application of the principle of empathy. In the process of Code Review, Developer and Reviewer may have different opinions on some codes, which is quite reasonable because different people have different perspectives and contexts. Discussion is also encouraged. But we should try to avoid conflict. So how do you avoid conflict, and how do you resolve conflict when it does occur? Different operations are recommended for different roles.

The Developer:

Keep in mind that the goal of Code Review is to improve the quality of the Code base and product. Each Reviewer’s suggestion or criticism is not personal to you; it is an attempt to help you, the code base, and the company as a whole. As a Developer I should think as much as possible, what constructive information is this comment trying to give me?

Whatever you do, don’t respond to review reviews with emotion. This will only lead to unnecessary conflict. If you’re really angry, take a break from your computer screen and come back to respond politely when it calms down. If there is a Reviewer with an impolite comment or even a personal attack, contact him or her in person or via email. If that doesn’t work, refer the matter to your supervisor.

The Reviewer:

Don’t get personal. Sometimes a Reviewer is not really a personal attack, but just a choice of words. I’ve already covered how to write good reviews, but when writing reviews and discussing them with Developer, it’s important to communicate them politely and based on technical facts to avoid upsetting the other person. Don’t be complacent about who is right just because you were Reviewer. The Developer, as a code Reviewer, has a better grasp of context information and may have written it better. This does not mean a Reviewer will compromise, of course. The core principle of continuous improvement of the code base is the bottom line and should not be broken unless there is an emergency (the last section explains what an emergency is).

A final point for Reviewer advice is that if your discussion leads to the conclusion that the code should be further optimized, then again, don’t delay optimization unless it’s an emergency, because once it’s delayed it will almost never change.

If the two parties are still unable to reach a conclusion after friendly discussion, invite more developers to participate in the discussion, or bring a higher level developer into the game and let him decide which approach is more reasonable and acceptable.

Iv. What is an emergency

There are exceptions to many things. The Code Review principles and recommendations mentioned above are applicable to most scenarios in daily work, but some flexibility may be required in an emergency situation. However, we need a clear definition of an emergency, or we risk compromising the health of the code for “emergencies” that are not urgent.

What is an emergency?

• Production accidents (P0, P1, etc.) occurred online, which significantly affected the normal use of users.
• A major security risk has been found online, and the vulnerability needs to be repaired urgently.
• A small change to avoid a rollback after a major release.
• There are hard deadlines that have been promised to partners.
• .

The list can continue to grow, but one general principle needs to be kept in mind: if this matter is not dealt with quickly there will be disastrous consequences. So, these seemingly urgent things aren’t really emergencies:

• The product manager says launch by a certain date (but there is no special reason for that date, it’s not a hard deadline).
• Developer has spent a lot of time on this feature and wants to merge the code as soon as possible.
• Today is Thursday, the last release date, and Developer wants to release on that date.
• .

The list could go much longer. But they don’t have catastrophic consequences, so you can’t compromise code quality for these “emergent” situations.

The above are the principles, methods and insights of Code Review based on Google’s engineering practice. If you have any ideas, please leave a comment.