13 Practices for Better Code Reviews

From Small Change Lists, to Effective Comments, and Prioritizing Review over Coding

Over the years, I’ve been on both the receiving and the giving end of code-review sessions. Done incorrectly, code review can be irritating, excessively time consuming, and have little or no impact on code quality. But, if done correctly, it can improve code quality and reduce the time spent delivering features. It also helps find tricky bugs and speeds up knowledge sharing in the team. The conversations that happen during code review can also lead us to new insights about the problem or alternative solutions.

I’ve put together a baker’s dozen of best practices you and your team can use to make code reviews more effective.                   

Create Small Change Lists

You’ve probably heard about the benefits of incremental development by deploying minimal features. Such a deployment reduces lead time, provides fast feedback and lowers technical risk.  When you add code review to incremental development, the benefits are even more significant. When the size of a change list (CL) increases, the time spent waiting for reviewer's feedback also increases, and most of the time it grows superlinearly. There are several reasons for that.

First, when there's a design flaw in a large CL, it can affect more code segments that you’ve implemented. But, if you break the large CL into several smaller ones and merge them one after another, you can avoid some reworks. The second challenge is merging your CL with the master branch. While you're working on your CL, other people are working on theirs. So, it’s only natural that a few CLs are merged into the master branch while your code is being reviewed. If the other CLs have some aspects in common with yours, the merge incorporates further changes, which cause extra review time. When a CL stays open for a long time, it annoys both the developer and the reviewer. It demoralizes them because there is no sense of progress. The job is not considered "done," and doesn’t show any profit for the business.

But, it’s not always easy to break a CL into smaller CLs. Major API changes, migration to a new framework or infrastructure, architectural refactors, and features that should be released together are just some of the challenges we face. Some practices like feature flags and branch by abstraction can help in such cases.

To use these methods, you may need to introduce toggle flags in the code, and create mock objects and extra interfaces. Although you may think it’s a waste of time to do all of these, most of the time it pays you back, by reducing lead time and helping you make smooth and reliable progress.

Change Lists Should Be Complete on Their Own

How you break your CL is very important. For example, it’s bad practice to divide a CL by putting off its unit tests. Although it’s a very easy choice to make, this approach will put the code base in a non-reliable state. This state can remain in the code, or even become worse when you approach a deadline and new features get prioritized over delayed works. So, you should try to have a small-but-complete piece of work, that includes its tests, and even its deployment/migration scripts.

The Best Argument Should Win

In the case of disagreement, which side should win? The coder or the reviewer? Whoever is more experienced? No one? In fact, it’s not a question of whom! Aim for a culture where  the best argument wins.

If you’re an experienced developer, you should be able to provide reasons for your argument, instead of just expressing your "feelings." If you have experience with something, try to describe that experience and why you think it’s applicable to the current context. So, in a best-argument culture, "I only make a claim if I have an argument to back it up!" Try this and you’ll benefit for two reasons:

• You won’t upset your colleagues over things you don’t have an argument for.
• Being ready to explain your argument empowers you. It encourages you to refer to textbooks, search your thesis online, ask other experts and senior colleagues. In the end, you’ll either ensure your viewpoint, or correct your assumptions.

You will learn something. So every time you add a comment, explain your reasons unless it is obvious or it has been previously explained. If you’re not as experienced as your colleagues, it’s your right and you should be encouraged to ask "why" if a comment’s purpose is not clear. Comments are a learning opportunity. You should have fewer repetitive flaws on the next change list. You can give similar comments to other colleagues when it’s your turn to review. The way to do this is to ask "why" when you don’t know.

Don’t hesitate to speak about the pros and cons of another alternative with your reviewer, if you think it’s better. Then, let the better arguments — the solution with more advantages — win. But, be ready to accept it whether it’s yours or not. In fact, this is good advice for  seniors and juniors — coders and reviewers. Be a passionate person who wants to know "why," a person eager to argue for your thoughts, but also open to other possibilities and solutions.

If the Other Solution is as Good as Yours, Skip the Argument

As an implementer, if you get a comment to rename a variable but think the suggested names are similar, with no clear difference: accept it. As a reviewer, if you want to suggest a change, but you cannot explain a clear advantage for your suggestion: skip it. You may think, "My solution is as good as my peer's solution. Why should I retreat?" The answer is clear. Your assumption is wrong. What seems equally good to you, may not be true for your teammate. If in your weighting system, the options are equivalent, you are the one who can tolerate it and show flexibility. So do it. Save the debate for the cases that matter to you.

Also, never keep count, "I retreated two times; now it’s their turn." The counting game is a destructive approach, doomed to failure. Two people won’t have the same sense of the numbers. They may count some items they are concerned about, but you have missed, and vice versa.

Don’t Complain in a Comment

In comments and comment responses, don’t complain or blame, just append your reasoning if it’s not clear. Commenting can be a hard situation on its own. You are going to disagree with a teammate; you are going to catch a problem in their work. So don’t make it even harder by complaining.

When your teammate reads your note, they may not read it with the same tone and strength you intended. If it’s a negative sentence, it’s not a surprise if they read it as a shout in their face or as it was written with total contempt. Emoji icons can help, but it’s difficult to show both seriousness and respectfulness with an emoji!

Remove words that convey negative feelings. But, don’t hesitate to show positive feelings when you see an awesome design or smart idea.

Don’t Say NOPE in a Comment Response

Respect the comments you receive. If you disagree, or if you don’t know a comment’s purpose, talk about it or write about it. But, no matter who you are, there is no room for NOPE — a response that indicates you aren’t going to accept a comment, and won’t discuss why. NOPE kills team culture.

Take Advantage of Oral Conversations

Using a review tool doesn’t bind you to just written communication. Oral conversations help you:

• Solve tough and challenging issues — such conversations are more interactive and powerful. You can avoid lengthy rounds of written arguments over the same comment that just annoy both sides.
• Avoid further rounds of review when there are a few issues remaining you don’t agree with. Ask the reviewer to sit beside you and resolve these issues.     
• Save time getting the big picture at the beginning of the review process. If you aren’t familiar with the purpose and design of a CL, you can save time if you ask the implementer to describe it orally.

Review Everything

You can’t be a good reviewer if you aren’t a good developer. During development, you consider many aspects: compliance with requirements, compliance with the architecture, compliance with the code style and conventions, compliance with previous designs, simplicity in design and structures, readability, avoiding redundant codes, low coupling, high cohesion, clear and consistent names, security, scalability, high availability,  etc.

There are many things to consider, and you should also consider all of them in review. So, review skills contain all the development skills and a few more — communication, clear reasoning, listening, and teaching.

Let Tools Take on Some of the Burden

As you’ve seen in the previous topics, there are many aspects to review. But, there are also many source-code analysis tools that can help take some of the burden off the reviewer. Tools are especially good at checking code style and convention. They can also detect simple bugs and security issues. If you hand off the simple and straightforward tasks to automation tools, you can focus on deeper, more challenging issues. Recently, I used SonarQube, one of the most popular analyzer tools. Such a tool can’t find every issue or provide every kind of creative feedback a skilled reviewer can. In my experience, it does less than 5 percent of what’s needed,  but it still really helps the reviewer by automating some parts of the process. When reviewers know that little code style issues, compiler warnings, and several known bugs will be checked, they can focus on more brilliant improvements. In addition, tools are better at routine work. A tool doesn’t get annoyed if it checks a single rule against thousands of variables, conditions, and loop statements.

Reviewers Should Be Certified

Can everyone on development team be a reviewer? No! I think technical leaders should certify people for review. A review certificate shows that a developer has mastered both the technical skills and business aspects of the product. Some people argue that when the coder is an experienced person, we can let a mediocre or less experienced person review the code. I don’t agree, because I see the review process as a control gate — a watch system to catch things the implementer missed. How can we encourage people to respect review feedback, if we don’t respect the review process itself. The review is an opportunity to encourage junior developers to master their skills. By pursuing the greatest diligence, a review badge should make a developer proud of their achievement, and start reviewing with confidence.

Prioritize Review Over Coding

You have your own CL to develop and another CL to review. Which one are you going to prioritize? Maybe doing code review is not on your task list. Maybe the issue is assigned to someone else on the tracking system and you don’t see your name on it. Either way, it's someone else's problem and you're not directly responsible for that CL.You might say, "I'll finish what I'm directly responsible for before reviewing the other CL. I’ll be mostly be assessed on my own CLs."

But, this approach is wrong. Prioritize review over code to reduce the work in progress (WIP). Usually, a developer starts a new CL when the previous CL is pending feedback. Having two at a time, one waiting for review and one under development, is a reasonable decision. But, this is often not the end of the story. If the reviewer doesn’t prioritize the review task, their poor teammate may have to start a third and fourth CL. At a team level, that means more open CLs. More open CLs mean the team will spend more time resolving conflicts, more overhead from context switches, and finally more delay to get them productive and profitable, and to get feedback from users.

On the other hand, prioritizing review helps team culture. We are all members of a team with a single purpose — benefitting our customers and ourselves. In such a culture, everyone is assessed based on their cooperation toward that common purpose.

Prioritizing review is a rule, but not a rigid one. Certainly, when there's only a small amount of work remaining in your own, or when your CL has a higher priority, the right thing is to continue on your CL. But, at other times it is highly advantageous to prioritize review.

The Mediator Should Follow the Same Culture

Imagine an argument between reviewer and the coder. Both sides have discussed their points, but they can’t converge to the same solution. One side says there is an advantage or disadvantage to something; the other side either does not accept it or thinks there are other important aspects that lead to different conclusions.

Even when both sides are willing to be flexible when they think both solutions are equally good, it’s possible the two sides will still insist on their opinions. This is a case where incorporating a third person, a mediator, will help. The key is how we see this mediator’s contribution. As always, "arguments should win," not individuals. The mediator should try to focus on the arguments, reasonings, the points, and their weights. We should not see this person simply  as a  voter, or someone  with veto power. Instead, we should see this mediator as a teammate, who can help us reach a clearer image of the subject. Using this approach, both sides can be "mostly" satisfied with the conclusion which appears out of thinking together. Sometimes, the final solution is one that was not even considered at first.

Pair With the Reviewer

Pursuing the practice of code review, doesn’t preclude pair programming. Pair programming has proven its usefulness, especially when it is hard to reach a good design or to find a solution. In these cases, you can ask for help from any teammates, but one of the best options is to pair up with the reviewer of the task. In addition to the help you receive, you also minimize time spent and review conflicts.

Such pairing does not need to be continued for the duration of the CL. It can be on a by-request basis. So, when you are in doubt about some aspects of your implementation, or when you are not satisfied with your current implementation, you don’t continue with the questionable approach. Instead of waiting for the formal feedback of the review, ask for it sooner.

Effective and Enjoyable Code Reviews

Pursuing good practices like the ones I’ve explored here for both technical (code) and cultural (communication) aspects can make your code reviews more effective and enjoyable. It helps code quality, knowledge sharing, and team culture.

About the Author

Mohammad Ali Bozorgzadeh is a technical lead, passionate about coaching developers and improving technical maturity in organizations.His technical expertise focuses on BigData, agile methodologies and continuous delivery. He is currently working at Sahab Pardaz, a company providing large scale and extra large scale BigData solutions in the middle east.