A Parable

John spent days writing a software component. He tested and double-checked his code, and he was satisfied that it worked properly, according to the requirements he was given, so he checked it into source control. A few weeks later, a new version of the software that included his code was released to production. A user discovered a bug caused by John's changes. The user tweeted about the bug, and this was retweeted thousands of times. Before long, word got back to John. An edge case that John had not considered was causing problems in production. He fixed the bug and checked his changes back into source control. And he waited. Hoping for the best.

June spent days writing a software component. She tested and double-checked her code, and she was satisfied that it worked properly, according to the requirements she was given, so she checked it into source control. June's team had a policy that required a code review prior to merging any code with the main branch. During the code review process, one of June's peers pointed out a bug in her code. It was an edge case that June had not considered. She fixed the bug and checked her changes back into source control. The code was reviewed again and merged with the main branch. June slept well that night.

The story of June and John illustrates some of the advantages of code reviews. Catching June's bug during a code review resulted in a faster and cheaper fix and resulted in less public embarrassment than catching John's bug in production. The two bugs were of equal severity, but one was less costly to fix.

What is a Code Review?

Why do we do code reviews? They take up time that could be spent writing code, designing features, or otherwise directly driving forward a project, so there is a cost. The answer is that the benefits of a good code review far outweigh the costs.

When I think of a code review, I think of a formal process in which one person reviews code written by another and provides written or oral feedback to the author, approving that code only after they deem it acceptable.

There are two parties in a code review: The Code Author and the Code Reviewer.

The steps in a code review are:

1. The Code Author makes changes to an application and checks those changes into a code repository
2. The author sends a description of changes to the Code Reviewer. The 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
3. The Code Reviewer retrieves the code, examines it, and (if necessary) provides feedback on changes that the author must make before the code can be merged with the main branch
4. If the code requires changes, the Reviewer sends the feedback to the Author
5. The Author responds to the feedback and makes any necessary changes
6. The Author re-sends to the Reviewer the code with these updates
7. Steps 3-6 are repeated until no more changes are required
8. When no more changes are required, the Reviewer approves the changeset
9. The code is merged with the main branch in the repository

Code Review Goals

A good code review will accomplish the following:

• Validate code
• Help make engineering decisions
• Share knowledge
• Increase code ownership

Let's discuss each of these goals.

Validate code

The most obvious reason to review code is to validate that it does what it is supposed to do. Generally, we look at this from an external point of view. For example, if we provide a given set of inputs to a function, we verify that the function returns the expected output. We can pull the code from source control and make sure it compiles and runs successfully. We can execute automated tests and validate that they all pass.

But we also want to validate the code from an internal point of view. If our team has coding standards, does the code adhere to those standards? While reviewing code, the reviewer looks for and calls out potential problems. Even if the code works well, there may be potential areas for improvement. For example, the reviewer may suggest ways to make the code more efficient, faster, or more readable. The reviewer should point out these as well.

Help make engineering decisions

Sometimes, a code review can drive engineering decisions. If there is confusion or inconsistency about how the application is accessing data or dividing services or testing code, code reviews can raise these issues and prompt a discussion. If different developers have different coding standards, it may indicate a gap in the team's standards and drive discussion around this.

Effective teams have published a set of coding guidelines that may describe everything from naming conventions to required test coverage. Developers must be aware of these guidelines and make an effort to adhere to them; but often non-compliant code slips through. A code review is a good place to catch this before the code is committed to the main branch.

Sharing knowledge

Another benefit of Code Reviews is that it allows sharing of knowledge.

There are two parties involved in a code review process: the code author and the code reviewer. By reviewing the code, the reviewer has a chance to improve the code itself and to address any weaknesses or knowledge gaps in the developer. Similarly, the reviewer can address his or her own weaknesses by seeing someone else's approach to a coding challenge.

The reviewer gains knowledge about a part of the system that someone else wrote. By reading the code, they may also learn something about the language in which it was written; or about a framework or a design pattern or an algorithm implemented by the author; or about the business cases and requirements of the application.

In addition, the code author can learn by reading feedback from the reviewer, who may suggest improvements that the coder did not consider.

Increase code ownership

I have worked on too many systems in which one developer possessed all knowledge about a part of that system. Confusion reigned when that developer left the team. No one understood how to maintain the orphaned code. By conducting regular code reviews, team members have a chance to understand parts of the system on which they are not actively working. This shared knowledge benefits the whole team, allowing flexibility in staffing and removing the danger of all knowledge departing when a team member departs.

Catch Problems Sooner

The Shift-Left Principle states that the cost of fixing a problem in software goes up exponentially as time goes on. Regular code reviews allow us to find and fix issues earlier in the development life cycle. Fixing them during the internal review process is much cheaper (and less embarassing) than fixing them after the code goes to production.

This does not imply that the Code Review process is the furthest left we can shift our error checking. By implementing pair programming or mob programming, we are likely to find issues even sooner. This also does not mean we should eliminate testing downstream from the code review process. No step is perfect and errors can always slip through. Still, a good code review process can be an important part of increasing software quality.

Code Review Process

The process of a code review is simple: The author checks code changes into a repository and announces that it is available for review. A reviewer looks at and runs the code and provides feedback. This feedback can be either written or verbal. Most Application Lifecycle Management systems (e.g., GitHub and Azure DevOps) support this process through a Pull Request. In these systems, the code of a Pull Request does not get merged into the main branch until one or more reviewers have approved it. We can configure these systems, setting specific rules about who must approve code before it is merged.

This process works best when everyone involved believes in it and considers code review time to be well-spent. Support from upper management can help encourage the process; but, public buy-in from the team's most respected developers is an even more effective way to get others to buy into this process.

Making It Better

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. I described the code review process above and listed reasons why it is worth the time and effort to do them. Let’s now discuss things that each party can do to improve the code review process.

How Can a Code Reviewer 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.

How Can a Code Author Improve the Code Review Process?

Test Your Code

This should go without saying, but you should always verify that the code works before submitting it for review.

Spend time validating that your code works. Manually run your code. Write automated tests and run them as you make changes to your code. Vary the inputs and consider edge cases and unexpected user actions as you do. A small change can break things unexpectedly and automated tests are great insurance against this.

Become a Reviewer

Code Reviews take time and effort, and you should respect the time and effort that the Reviewer commits to the process. A final scan of your code often reveals obvious problems, such as spelling errors and redundant or unnecessary code. It can even reveal more fundamental problems, such as a bug you missed on the first pass. Taking a few minutes to review your code reduces the time and effort required by the Reviewer. As a bonus, your code will look better to the Reviewer, making them more efficient and encouraging a better relationship.

Communicate clearly

A Pull Request consists of a set of changes to the code. It should always contain a description of those changes. If you write a clear description of those changes, the Reviewer will know what to look for and their feedback will be more useful.

When responding to feedback, always communicate what you changed in response to that feedback. This will give the Reviewer an idea of what to look for and make it easier for them to read and evaluate your changes.

If anything is unclear in the feedback, solicit more information - either through comments in the PR, via email, or with a verbal conversation. Written communication is sometimes flawed and requires clarification.

Use the computer

As mentioned earlier, you should test your code before submitting it to a reviewer. Much of this can be done automatically using the computer: Compile the code; run all automated unit tests; and use a linter to check the code style against a set of pre-established rules.

Answer questions with code

The clearer you make your code, the easier it will be to understand. Code comments can be useful, but you must take care to always keep them up to date with the code. The best way to clarify your code is to make it self-documenting. Well-written, self-documenting code will almost always communicate its intent better than code comments.

Spend some time refactoring your code to make it more readable. Here are some examples:

If you have lines of code that perform a property tax calculation, consider putting this into a method with a name like "CalculatePropertyTax". Calling this method is probably much clearer than trying to understand what the calculations do.

If you have a number or code with a specific meaning (for example, a tax rate or department id), assign that value to a constant, a variable, or an enum. It is much easier to read and understand this:

var taxDue = revenue * TAX_RATE

than this:

var taxDue = revenue * 0.23

Your goal should be to make the code as readable as possible. Consider questions the Reviewer might have and strive to make the code answer those questions.

Keep it Simple

I have seen too many Pull Requests that make a plethora of changes. It is best to create a Pull Request that only makes one change to the system (although you may choose to implement that change using multiple functions). If you are adding a new feature and fixing a bug, split these into two PRs. If you are changing two distinct parts of the system, split these into two PRs. Large PRs are confusing and overwhelming. The time and effort to review two PRs is almost always less than the time to review one large PR.

Breaking up large changesets can narrow the scope of your change, making each one easier to understand and review.

Sometimes, we create Pull Requests for non-functional changes, such as changing the formatting of our code. These often affect every line in a file. These should always be submitted as their own changeset. If we combine these changes with functional changes, it makes it nearly impossible to determine which lines had a functional change.

Stay Cool

Code reviews can be a source of conflict. Code Authors often feel an ownership of the code they write to the point that they perceive any criticism of their code as a criticism of themselves. Avoid this outlook. Separate yourself from your code and do not take constructive feedback personally. It will be better for your mental health, and it will allow you to look more objectively at how you can improve your code. Respond graciously to the Reviewer's feedback. You both have the same goal: to improve the quality of the codebase. Keeping your cool can be especially difficult when you know the Reviewer is mistaken. Reviewers are human and they are allowed to be wrong sometimes and you should be patient when this happens. Consider that a lack of clarity in your code may be a source of a Reviewer's mistake and strive to address this.

Recognize there are multiple right ways to do almost everything in software development. If you are both correct, it will save time and effort to skip the debate and agree with the Reviewer.

Respond quickly

This is similar to advice to the Code Reviewer. Delaying the time between receiving feedback and responding/working on the feedback slows down the entire process. The sooner you work on changes, the fresher will be the original code in your mind. The sooner you re-submit your changes, the more fresh will be the feedback in the mind of the Reviewer. This can be a challenge if you have begun some other work; but recognize that there are significant benefits to responding quickly to code review feedback.

How Can a Manager Improve the Code Review Process?

Among a manager’s responsibilities are training their employees, delivering a high-quality product, and establishing the team culture.

We have established that code reviews can improve code and product quality. Using tools like GitHub and Azure DevOps, management can establish rules that require pull requests and code reviews before any code is merged with the main branch.

By requiring each code author to sometimes serve as a reviewer, they can disseminate knowledge of the business rules, the code, and the technology throughout the team, avoiding isolated silos of information. I recommend that reviewers rotate whose code they are reviewing. If Joe always reviews Jean’s code, Jean will not get the perspective of others on the team.

Most importantly, they can push for a culture that fosters cooperation and emphasizes that code reviews are not adversarial, but are an opportunity for code authors and reviewers to work toward a common goal. Organizations tend to work best and achieve more when everyone cooperates.

Conclusion

A Code Review is no guarantee of quality software. Careful coding, hiring qualified developers, and automated testing also contribute to the quality of your end product. But a good Code Review process is a good step in making everything better. Code Reviews have become an important part of most of the projects on which I work, yet I remember a time before I even knew such a thing existed.

These days, code reviews are almost ubiquitous on my software projects. They help us address weaknesses among developers and reviewers, enforce compliance with coding standards, and improve the quality of our codebase. They map well to the fast feedback loop emphasized by many agile methodologies.

If we can catch bugs before they go into production, we can save ourselves embarrassment, time, and money. A good code review process helps us achieve that.

There are challenges to a good code review, but these challenges can be addressed with a bit of effort on the part of both the author and the reviewer.

---

Note: Some of the ideas in this article were drawn from the following articles by Michael Lynch: "How to Make Your Code Reviewer Fall in Love with You" and "How to Do Code Reviews Like a Human". Derivation of these ideas are used under the CC BY 4.0 License.