5 lessons learned from code review

We often hear team members say:

"Code review on this project is a waste of time."

"I don't have time to do code review."

"The launch was delayed because of my mean colleague My code hasn’t been reviewed yet.”

“Can you believe that my colleague asked me to change my code? Why do we need to change such an elegant and perfect code?”

Censorship?

　One of the most important goals of any professional software developer is to continuously improve the quality of their work. But only through teamwork can we focus our efforts in one place and improve software quality. Code reviews are one of the most important ways to achieve this goal. In particular, code reviews can:

Discover defects and better solutions from another perspective.

Make sure there is at least one other person familiar with your code.

Help train new employees by going through senior developers’ code.

Promote knowledge sharing.

Motivate developers to write better code and solve problems in the code to avoid being caught by others during review. 　

　Code review must be thorough

　 However, unless you can actually spend time and energy on code review thoroughly, the above goals will be difficult to achieve.

My opinion is that about 25% of original development time should be spent on code review. For example, if it takes a developer two days to implement a program, it should take about four hours to review.

Of course time is not the most important thing, the key is whether you can review the code correctly. You must understand the code you are reviewing. This means that you not only have to know its syntax, but you must also understand how the code fits into the context of the application and becomes part of a component or library. If you can't grasp the meaning of each line of code, then your review will not be adequate and will not be very valuable. This is why well-executed code reviews are mostly impossible to complete quickly: because we need time to study the various codes that trigger a given function to ensure that the third-party API is used correctly.

When reviewing, in addition to looking for code flaws and other issues, you should also make sure to:

Include all necessary tests.

Appropriate design documentation has been written.

Even developers who are good at writing tests and documentation will forget to update the code when they change it. Code reviews should ensure that this information does not become useless over time.

Avoid excessive code reviews

Developers should work hard to clear the backlog of review tasks. One approach is to do code reviews in the morning and get the review done before starting your own development work. Of course you can also review code before or after lunch or at the end of the day. All in all, you should treat code as part of your daily work, not as a burden, so you should avoid:

No time to deal with a backlog of review tasks.

Delay in release due to incomplete review.

I foolishly review the irrelevant code, but it has been changed beyond recognition after being handed over to you.

I went through the motions in a hurry because of time constraints.

Write reviewable code

　When the code backlog gets out of control, the reviewer is not the only one who needs to be held responsible. For example, if your colleague spent a week adding messy code to a large program, the released patch will become difficult to review, there will be too much content to understand and delve into. Even the purpose and basic structure of the code are unclear. This is not what writing code does.

　Before writing reviewable code, you need to do some preparation. If you need to make some difficult architectural decisions, it's best to discuss them with the reviewer first. This will make your code easier to review and understand, because they will already know in advance what you want to achieve and how you plan to achieve it. This also prevents you from having to rewrite a large chunk of your code if a reviewer later comes up with a completely different and better approach.

　The project architecture should be described in detail in the design document. This is important because it allows new project staff to understand the existing code base more quickly, and it also helps reviewers do their jobs better. Additionally, unit testing allows reviewers to better understand the usage of individual components.

　If your patch also contains third-party code, submit it separately. Just imagine, if 9,000 lines of jQuery were inserted in the middle of the code, would it greatly increase the difficulty of review?

　One of the most important steps in creating reviewable code is to annotate your code reviews. This requires pre-reviewing it yourself and then adding comments where you think it will help the reviewer understand. I've found that code reviews after comments take relatively little time (usually just a few minutes). Of course, code comments should still be used where appropriate. In addition, research shows that developers themselves will find many existing defects when commenting on code.

Code Refactoring

Sometimes, we have to refactor the code base. If you happen to encounter a large application, it may take several days (or more) and generate a large number of patches. In this case, it may be impractical to implement a standard process for code review.

　The best solution is to refactor the code step by step. First give a reasonable scope, determine the corresponding code base, and then make rectification and reconstruction in the target direction. After the first part is done, review and publish it, then refactor the second part... until it's all done. This staged approach may not always be possible, but if we use this approach when thinking and planning, we can avoid large-scale monolithic patches when refactoring. Of course, this approach may require more refactoring time, but it will also produce higher quality code and an easier review process.

　 If incremental refactoring of the code is still not feasible, then another solution is pair programming.

Dispute Resolution

There is no doubt that every member of the team is a talent, but this can also easily lead to differences of opinion when faced with specific coding issues. As developers, we should keep an open mind and be willing to accept different opinions from reviewers.

　As a reviewer, you should speak tactfully. Before making a suggestion, consider whether your opinion is truly better or just a matter of different taste. If you choose an area of ​​code that really needs improvement, the whole convincing process will be much simpler. And the words should be said like, "It's worth considering here...", "Someone suggested...", rather than "The algorithm I wrote with my eyes closed can be more efficient than yours."