Code Reviews

A Code Review can sometimes be a painful process. Software developers often feel a personal attachment to their code and may feel that criticism of their code is a criticism of themselves. Reviewing code takes time and attention and human beings do not have an unlimited supply of either.

The good news is that it does not have to be that way. There are things we can do to make a code review less painful.

There are two parties in a code review: The Author and the Reviewer. In my last article, I described the code review process and listed reasons why it is worth the time and effort to do them. To summarize, a Code Review begins when an  Author sends to a Reviewer a list of changes they made to an application. The Reviewer then looks at the code and either approves it or makes suggestions and sends it back to the Author. The process repeats until the Reviewer approves the changes.

The author's changes are known as a "Change Set"; the description of those changes is a "Change List"; many Application Lifecycle Management tools (e.g., GitHub and Azure DevOps) support a "Pull Request", which combines the two with the source code and is a formal entry point for the Reviewer to begin reviewing the code changes. I often use these three terms interchangeably because they are so closely related.

In this article, I will describe ways that the Code Reviewer can improve the Code Review process.

Do Not Wait to Review

If you can begin reviewing a Pull Request immediately, it saves a lot of time and trouble. The sooner you begin, the sooner you can return any feedback and the better the chance that the code will still be fresh in the mind of the author.

In addition, it is likely the author will begin making other changes to the system after submitting a Pull Request. It is usually easier to merge code if fewer changes exist between the two branches, so a quicker turnaround makes code merges easier.

Beginning your review also shows respect for the process and for the code written by the author, improving your relationships.

Work High Level to Low Level

Always begin your evaluation by looking at high-level decisions in the code, such as class structures and method interfaces. Often, changes made at this level will address issues at a lower level, such as the implementation of the business logic.

Use the Computer where you can

You can use a computer to automate many of the mundane tasks of a code review. The computer can compile the code and run all the unit tests. A linter can validate that the style is correct.

Create a Style Guide

Speaking of style, many organizations adopt a set of style guidelines to which they expect all code to adhere. The guidelines chosen are less important than the fact that everyone is using the same style, so that code is consistent and easy to read.

You can create your own guide from scratch or start with an existing one, such as Google's style guide (google.github.io/styleguide)

Arguing over code style is usually a waste of time. Simply point to the style guide to settle any arguments. If the style guide does not cover a case, modify it to include this. This guide can evolve over time, as the team writes code and discovers areas of ambiguity in the guide.

Include Code Examples

A code example is a nice way to communicate a change to an author. This is particularly true if you suggest a pattern with which the author may be unfamiliar.

Do Not Make It Personal

Programmers often take great pride in their work and sometimes internalize critical feedback, as if criticism of the code equates to criticism of the coder.

You cannot control the way the code author thinks, but you can minimize this feeling of attack by avoiding anything personal in your feedback. If you find yourself writing "You" in your comments (for example, "You need to initialize this variable"), consider replacing it with "We".

You can soften your feedback by using a passive voice (this is one of the few times I will recommend the passive voice in written communication). For example, instead of saying "You should split this into two functions", say "This function should be split into two functions".

Another way to soften feedback is to phrase it as a request, rather than a command. For example, "Can we make this function private?" is less confrontational than "Make this function private" and is less likely to trigger a defensive reaction.

Finally, avoid advice based solely on your opinion. Instead, cite a software principal to support a suggested change. "I think this should be split into two classes" is less compelling than "This class does two different things, which violates the Single Responsibility Principle. Consider splitting it into two classes".

Prefix review comments with labels:

I recently read about a team that adds a prefix to many of their feedback comments. Here are some suggestions.

Issue: A problem with the code that needs to be addressed
Suggestion: A possible way of addressing an issue
Question: A request for clarification. Useful if the reviewer is unsure if something is an issue
Nit: A trivial change
Thought: An idea for improving the code that the author may or may not choose to implement
Praise: Point out something good in the code

You can read more about this idea here.

Praise Sincerely

I love the idea of adding praise in the reviewer's feedback. So often, we think of feedback as only negative, but it is also a chance to call out something positive.

Goal: Improve Code

Some reviewers insist that every issue needs to be fixed before they approve a PR. This can lead to very long review cycles and bitter feelings between the author and reviewer.

There are a few ways to avoid these long review cycles.

One way is to set a goal to improve the code, rather than to make it perfect. Blogger Michael Lynch uses the phrase "Aim to bring the code up a letter grade or two" to describe this. If we receive code that is a "C" grade, and we can bump it up to a "B", that is a win, even if there are still issues to be addressed. Chances are the code author will learn something from the feedback and their next set of code will start closer to a "B", making it easier to move it to an "A" in a review. Of course, we want to prioritize the most critical issues to fix.

If only trivial fixes remain in a PR, it is OK to approve it.

Finally, if a PR contains a large number of changes, suggest splitting it into multiple PRs to make it more manageable. Suggesting where to split the code is very helpful in this case.

Avoid Repeating Feedback

It is not uncommon for the same issue to appear multiple times in the same PR. Do not waste time re-typing the same comment. A line like "See naming convention comment above" will suffice.

Consider the Scope of the Review

As a general rule, you should only review and provide feedback on those lines that the author changed. This helps to limit the review cycle.

There are some exceptions to this rule, in my opinion:

  • If a change affected a line that did not itself change (for example, the code in a method was modified and the name of the method no longer reflects what it does)
  • If a change is trivial and easy to fix (for example, you spot a spelling error in a line of code that did not change)

Stalemates

Sometimes, a Code Review process gets stuck as the Author and Reviewer argue over whether something needs to change. This can prevent the review from moving forward; but, it can also result in tension between the two parties, which may hinder future reviews.

When you recognize a stalemate occurs, the first step should be to discuss it verbally. Code Review communication is usually written, which can sometimes be misinterpreted. Walk over to the other party's desk or schedule a virtual call to talk about the conflict and how to resolve it.

For disagreements on fundamental design decisions, you may need to schedule a formal design review. This was likely something that was missed during the original design.

Consider whether your opinion is worth blocking a PR. Software development contains very little dogma and often there are multiple correct answers to the same problem. If the other party's solution will work, consider conceding your point.

As a last resort, you may need to escalate the conflict to an architect or manager and allow them to resolve it.

The last thing you want is for a Code Review to hold up a Pull Request merge indefinitely.

Conclusion

The benefits of Code Reviews almost always outweigh the costs. As a Code Reviewer, you can take action to reduce the cost of a Code Review in terms of time, effort, and stress.