What to Say About Code Review (CodeReview)

In a mature team, CodeReview is an indispensable step in the entire R&D process, and those teams that are about to mature may have many misunderstandings and questions about CodeReview, and they are not clear about how to do CodeReview. The author of this article will combine his own my experience and knowledge, and talk about some of my understanding and suggestions for the CodeReview process.

What is CodeReview

CodeReview is also called code review or code review in China, also referred to as CR, which means that in the process of software development, engineers review the code written by others (hereinafter collectively referred to as CodeReview) to achieve the purpose of controlling code quality. The usual process is initiated by the code writer. Others in the team are invited to review the code. Others make suggestions for improving the code, and then the code writer will modify and resubmit it until the code is reviewed by everyone.

Why do CodeReview?

In fact, many people do not pay much attention to CodeReview, because the effect of CodeReview is difficult to see in the short term and difficult to quantify. Just like exercise, occasional excessive exercise is not only detrimental to the body, it may also be counterproductive, but long-term persistence will definitely make you healthier, but it is difficult to quantify how much it can make you healthy, but some time ago, it exploded on github. Reference data is given in the project "Program Ape Life Extension Guide", 45 minutes of swinging exercise three times a week can reduce all-cause mortality by 47%, and according to its formula, it can increase life expectancy by about 9 years. In addition to increasing life expectancy, exercise can significantly reduce the incidence of many diseases. Sticking to CodeReview is like sticking to exercise, and the trend is definitely to make the entire codebase healthier and longer.

In the long run, CodeReview has several obvious benefits, which are discussed next.

Improve code quality

If a system is compared to a living body, and lines of code are compared to cells, bad designs are like cancer cells, which will gradually spread and eventually kill the system. The process of CodeReview is like T cells engulfing cancer cells to ensure the healthy growth of the system. Let the system have a longer vitality.

No one has reviewed the code, and its code level is the level of the person who wrote the code, and the code reviewed by a team, its level will be close to or even exceed the highest level of the entire team.

Because a single person may be better at certain conveniences, and by gathering everyone's strengths, they can be better at all aspects. In addition, with the normalization of the CodeReview process, the coding ability of each participant will gradually improve, infinitely approaching the highest level of the team, because in the process of CodeReview, you can see what others have done well, and you can learn Experience, you can also see where others are doing badly, and learn from it. Over time, it gradually builds up the abilities of the participants.

Identify problems early

When there is no CodeReview process, we all rely on testing, or even relying on user exposure after the function is launched. This method of discovery is too late, especially when users expose problems, the problem may already be very big. The later the problem is exposed, the greater the risk . CodeReview is generally placed before code testing. If problems can be found at this stage, various risks can be strangled in the cradle in advance. But, can CodeReview really catch problems in advance? If so, what kind of problems can be identified in advance?

I personally think that CodeReview can discover process or implementation problems in advance, especially when it comes to business requirements, people with different work experience will definitely have different understandings of the same requirement, and there will be big differences in code implementation. Sometimes a little bit of understanding deviation leads to the consequences of missing a thousand miles. People who write code will not be able to find problems because they are in their own thinking patterns, while others may because of their rich experience or the need to understand the relevant background. More, you can easily find the problem. This kind of situation usually occurs to newcomers in the team. They don’t know much about the existing system and business, and problems are easy to occur when implementing the requirements. At this time, if someone helps to point out, more serious consequences can be avoided, and it is also helpful for newcomers to understand Business and integration into the team.

Another problem in the CodeReview process is also relatively easy to find, that is, the pits that others have stepped on before. For example, Simple Date Format in Java actually has the problem of thread safety, and it is easy for people who don't understand it to step on the pit. If others have stepped on this pit, they can help you point it out in advance during the CodeReview process. Many open source class libraries, and even many packages in Jdk have more or less use pits... There are countless such examples.

Of course, CodeReview is not omnipotent, and there are many problems that cannot be found, such as some edge cases or the accuracy of code calculation results. For example, if your code implements the calculation of a very complex formula, you can't expect someone to find it by looking at the code. question. These are still honestly to do the test!

transfer of experience and knowledge

Programmers can write code that works, but only real engineers can write high-quality code with low bugs, easy expansion, and easy maintenance. More advanced engineers can also help others become qualified engineers. The CodeReview process is also the most direct way for senior engineers to help others.

In CodeReview, you can see excellent code written by others, excellent design, and even business background knowledge related to changes... The more you review, the more you learn. In addition, even if the code is not very good, after the review of others, it will inevitably leave a lot of comments or suggestions for changes, and even the pits that others have stepped on before, you can see it, and you can also learn from these contents. A lot of new knowledge. CodeReview is not only a way to improve code quality, it can also shoulder the functions of knowledge accumulation and message collection and distribution.

To give an example that I have encountered before, we have a function that needs to operate the files on the machine. Of course, it can also be implemented with Java File, but it is very inconvenient to operate multi-level folders, and we need to write a lot by ourselves. The code, if not good, is prone to bugs. Later, I found that the IOUtils class is provided in the apache-common package, which can easily manipulate folders. I wrote this in a CodeReview comment, and anyone who has seen it will know that this thing is available.

Therefore, please do not hesitate to leave your suggestions when reviewing other people's code. In addition, don't forget to pay attention to other people's suggestions when CodeReviewing, maybe you can learn new things.

How to do CodeReview

After talking about the benefits of CodeReview, I believe you must be eager to try it. How can you do CodeReview well? Here I share my opinion based on my knowledge and experience.

Steps for CodeReview

understand the background of the change

CodeReview doesn't look at the code as soon as you come up, so you may look in the clouds, which is a waste of time. Although CodeReview is a review code, first of all you need to know what kind of function the code you want to see realizes, and in what context. Better to review other people's code and find other people's problems.

Look at the big picture

After knowing the background, you will have a general coding idea and a main process line in your mind. At this time, there may be two situations. You and the code writer have the same ideas. Then you can follow your common ideas to help review whether the whole process is correct. Another situation is that you have different ideas. You have to look at the code to understand the writer's thinking, and then confirm whose thinking is wrong or whose thinking is better, and then work with the writer to optimize the process to a better one .

Layer-by-layer refinement

After the complete process is determined, you can gradually dive into the details of the code. There are many places where the details can be reviewed. You can see the content of the next section, which will not be expanded here.

CodeReview's focus

In the process of CodeReview, if there are some footholds, it can help you to complete the process of CodeReview better. I can summarize the following points:

• Functionality: Does the code perform as expected, does it implement all the required functionality?
• Complexity: Is the feature implementation too complex? Overly complex code is more prone to problems and less maintainable.
• Coding style: Does the code conform to team coding standards?
• Documentation & Comments: If the code function is changed, pay attention to whether the related documents and comments are changed synchronously. Incorrect annotations and documentation can incur understanding costs for future developers.
• Code highlight: Don't be stingy with your compliments if you see something good done in a change.

The above description is more general, let's give some more detailed examples to help you understand the above points.

• The code is well designed, readable and maintainable.
• Is there any thread safety bug.
• The code does not add unnecessary complexity.
• The developer did not write some code that is not currently in use, and this useless code will probably not be used in the future, so don't assume it.
• The code has proper unit tests.
• The test logic is well designed.
• The developers used clear and unambiguous names for variables and functions.
• Comments are clear and useful, and explain why , not just what .
• The code is well documented.
• The code style conforms to the specification.
• Is there a better implementation of the code? (performance, resource usage...)
• ...
  For more details, please refer to Google Engineering Practice - Code Review

Precautions

Courtesy of CodeReview

First of all, CodeReview is not a stage for you to show off your skills. You need to politely point out the problems of other people's code, and don't diss them. Secondly, CodeReview should not ask questions for the sake of asking questions. Some codes are just fine, and you don't need to worry about having to ask a question, just pass it directly. Here's an extra point. Most of the reviews of other people's code only focus on what others have done badly, while ignoring what others have done well. Don't forget to give a like when you encounter good code and good design.

CodeReview should be timely

The CodeReview mentioned by others should be dealt with in a timely manner. In many companies, CodeReview is a necessary part of the development process. Those who do not pass the Review will definitely not be able to go online. If the CodeReview is not processed for a long time, the subsequent process may be delayed. Another point, CodeReview is mutual, today you help others to review in time, and others will help you to review in time tomorrow.

How to write CodeReview friendly code

Programs are written for people to see, and they can run on machines. —Harold Abelson

Always keep the above statement in mind.

Do a self-examination before submitting

To deal with some low-level errors in advance, you can use some tools to complete some simple checks. For example, our team will use checkstyle, spotbug, sonar, pmd and other tools to complete the code style and some potential bug checks. Then do a self-test of the function yourself, eliminate some bugs as much as possible, and reduce some burdens for CodeReview.

Write a clear description of the change

This corresponds to the background of the changes in the CodeReview step above. As a code writer, you can't let others start with the code as soon as you come up. Many other people who look at your code may lack the context information related to the code changes and fully understand the code. The cost is very high, so this information needs to be clearly described in the change description, including but not limited to the change background, change points, process, specific design... In one sentence, a good change description should clearly state what changes were made this time. , and why .

A more detailed change description can reduce the understanding cost of the reviewer, and complete the CodeReview process faster, and your code changes can enter the subsequent process faster.

Keep individual changes as short as possible

A very big change has almost no review, because a big change is difficult to review at first, in fact, it is more time-consuming, and the probability of problems is also greater. Whoever reviews the code but has problems after going online may have to cooperate with You are the one to blame. Therefore, it is recommended that you break down large code changes as small as possible, because small changes have the following benefits:

• Faster code reviews It is easier for someone reviewing code to spend 5 minutes reviewing a series of small changes than 30 minutes reviewing a large change.
• Review is more thorough. After making larger changes, reviewers and authors tend to get frustrated by the back and forth of a large number of detailed comments that sometimes miss or miss important points.
• Reduce the possibility of causing bugs. Since you make fewer changes, it's easier for you and your reviewers to effectively infer the impact of the CL and see if it's causing a bug.
• Reduce Unnecessary Work When you write a huge change and the reviewers think you're in the wrong direction overall, it can cause you to waste a lot of work.
• It is easier to merge code because large changes can lead to a lot of conflicts, so it takes a lot of time to merge the code, and it may cause problems due to the merged code.
• Helps you make better designs Elegant designs and perfecting a small change is easier than a big one
• Make it easier for reviewers to review some changes without affecting your ability to continue coding.
• If something goes wrong, it's easier to roll back

For those changes that are difficult to split, it is not necessary to completely prohibit them. Sometimes the changes are very large and cannot be split anymore. At this time, it is recommended to use other methods to ensure code quality, such as full-process testing. Then it is also recommended to make a quick recovery plan in the event of a problem.

Precautions

Pay attention to your emotions

The author of the code is the weaker party in the process of CodeReview. Many people have a reluctance to accept the code when they receive a comment and are asked to modify the code. In fact, I often have this mentality myself. Especially when time is tight. In fact, this is a very normal mentality, but you still need to control your emotions. From the perspective of the commenter, he is definitely not making things difficult for you, but also hopes that the quality of the code will be higher. If this is the case, you can first discuss with the commenter whether his request is reasonable. However, if time is tight, negotiation can be left unmodified or modified later. There are indeed some icing on the cake modifications that do not need to block the subsequent process.

CodeReview process should be submitted as early as possible

Under normal circumstances, it is recommended to submit CodeReview as early as possible, and set aside a certain amount of time to make changes, so as not to make this process hurried as much as possible. The practice of most companies is to enter the CodeReview process at the same time as entering the testing process.

A few misunderstandings about CodeReview

Is CodeReview a pure waste of time?

If your team is just starting out with the CodeReview process, this question is sure to get asked a lot. So isn't it? Here, let me take the analogy of exercising. Everyone knows that exercising is good for health, but if it is just a whim, occasional excessive exercise is not only unhealthy for the body, but may also be counterproductive. In fact, the impact of persistent exercise on health is actually difficult to quantify, but we all know now that exercise is good. Similarly, CodeReview is like exercise, it may consume a certain amount of energy and time, but it will definitely make the whole code base, the whole system and even the team healthier if you stick to it for a long time.

In addition, if a team's CodeReview mechanism is very mature, and the code writers will gradually submit their code quality as the number of times they are reviewed increases, then the problems in the code will definitely be gradually reduced, followed by CodeReview. The process is getting easier.

The difficult path will become easier and the easy path will become more difficult.

The construction period is very tight, and there is no time to do CodeReview!

This is also an excuse for many teams not to do CodeReview. The time you save by not doing CodeReview will cost you more time for later testing and even longer-term maintenance. We have a well-known old saying called "sharpening a knife without accidentally chopping wood". In fact, CodeReview and testing are the work of "sharpening" in the software development process.

Do a good job of controlling beforehand rather than making up after the fact. Because the cost of making up for it will be very high,

Only senior engineers are qualified to review other people's code?

Although in fact almost all senior engineers are reviewing other people's code, I understand that the reason for this phenomenon is actually relatively simple. Senior engineers are indeed more experienced, and it is easier to review the problems in other people's code, so they will leave more There are many review records, creating the illusion that only senior engineers participate in CodeReview. But this does not mean that junior engineers are not qualified to review other people's code. The so-called three-person team must have my teacher. Even junior engineers may find problems in other people's code when they review.

Even if you are not experienced enough for the time being, it is difficult to find out other people's problems, but you can also learn a lot from the process of other people's review of code, as mentioned above, CodeReview is also a way of transmitting experience and knowledge. Participating in CodeReview is also an essential step in becoming a senior engineer.

There is a test process, why do CodeReview?

This question has already been answered above. CodeReview needs to look at many aspects, and testing can only guarantee the accuracy of its results.

With CodeReview there is no need for testing?

The person who asked this question went to the other extreme from the person who asked the last question, putting CodeReview and testing on the opposite side. Although CodeReview can indeed find some obvious problems in advance if it is done well, it is people who do CodeReview, and people cannot verify the execution process of the program on a large scale and accurately, and this is precisely what automated testing is good at. Yes, so CodeReview and testing are not antagonistic, but complementary.

As long as I implement the CodeReview process on my team, will the code quality rapidly improve?

First of all, the code quality will improve, but maybe not so fast. When I joined a team that did not have a CodeReview process to a team that had a strict CodeReview process, I had a long adjustment period. My biggest feeling at the time was that CodeReview was too time-consuming, especially when I had not yet adapted to the new team's development specifications. The time spent writing code and changing the code after being reviewed was basically 50 to 50 percent. Of course, it will be much better after that. . In addition, everyone also needs to spend about 20% of their time and energy reviewing other people's code, which is equivalent to spending nearly one-third of the development time on CodeReview-related work. This ratio is so high that your boss may doubt it, but we were It did, and the result is that our code is very high quality.

From my experience, if CodeReview wants to be effective, time investment is definitely indispensable, freezing three feet is not a day's cold, dripping a stone is not a day's work. Don't overestimate its short-term value and don't underestimate its long-term value.

References

1. Google Engineering Practice - Code Review

---

Today's sharing is all here, and everyone knows that the environment outside is not good, and at this time, we can't stop slack. Lewis Carroll uttered the following sentence through the mouth of the Queen of Hearts in "Alice in Wonderland", and it seems surprisingly appropriate now!

In our place, you have to keep running to stay where you are. If you want to get to another place, you must run at twice the current speed.
——【English】Lewis Carroll, "Alice in Wonderland"