Code reviewing for humans

Most software teams practice some sort of code reviews, either face to face, using GitHub and BitBucket pull requests, or using a tool specific for code reviewing like Gerrit. Study after study show that code reviews uncover bugs that wouldn't easily be caught otherwise.

In addition to finding bugs, code reviews can help in many other ways: mentorship and knowledge sharing between developers, ensuring commenting and formatting standards are followed, and enforcing architectural standards to name a few.

Unfortunately code reviews take time, and they are one of the first things to be skipped if the team is under pressure or a critical bug needs to be fixed. But, you should consider code reviews as an investment that will pay off dividends down the road by finding bugs before they hit production and ensuring your codebase is clean and your entire team understands it.

In this article, I am going to give you a guideline for how to give code reviews. Like most aspects of software development, the best way to get better at code reviewing is by doing it. Code reviewing is where a developer's technical skills collides with her interpersonal skills, where communication is as important as technical chops. Code reviewing is a human process, and at worst, egos are involved, feelings are hurt, and moral is destroyed. At best, it produces better code, more shared knowledge, and deep engagement from all developers. This is code reviewing for humans.

How to write code that is easy to review

It's difficult to review more than about 300 lines of code at a time, so try to keep your commits to about that length or less. Just like writing a proper class, function, or method, you should be thinking of the single responsibility principle, a single commit should narrowly encapsulate one piece of functionality and associated unit tests.

For example, your first commit might be to introduce a calculatePrice function that only handles basic prices, the next commit may be to add calls to that function from the various places that will need it, and further commits may introduce extra behavior, like handling discounts, taxes, and special offers.

Keep your commits small and self-consistent just like the rest of the code. The better you can do this, the better the feedback on your code will be.

Code reviewing for humans

Polite criticism can feel like an attack under the wrong conditions. For code reviews to be effective, the commit owner needs to feel safe and respected.

Create an environment that is non-threatening and collaborative

You should expect to spend some time performing code reviews. Spending 20 minutes looking at 300 lines of code is typical. Once the code author responds to your code review, it's important to return to the code and make sure that all of your comments were completely addressed, you can make any clarifications on things that were unclear, and you can continue the conversation on tricky spots that need it.

With that in mind, here are the guidelines that we follow at Ganchrow Scientific:

 Be humble

Too often, ego gets in the way of a good code review. Even the most senior developers and best code reviewers make bad suggestions. Often, they are neither correct nor incorrect, but simply a suggestion that just doesn't fit with the rest of the code.

For example, last week, I was reviewing some code to display prices in a webpage. The controversy was around this seemingly simple function

function roundDecimal(odds: number): String {
  return String(
    odds >= PRICE_EPSILON ? Number(odds.toFixed(SIG_DIGITS)) : 0
  );
}

Why, I asked, was the odds parameter first converted to a String, then to a Number, and then back to a String? It seemed like needless waste and complication. After a bit of back and forth, it was made clear to me that this was done elsewhere in the code and was to ensure that 12 becomes '12', and not '12.000'. Instead of digging in and requesting a rounding function that I liked, I allowed the change to go through.

The code author had more context and understanding of the change. Sometimes, even if you can't come to an agreement on a small issue, you should be prepared to let things slide.

The goal is to ship quality code. Ego getting in the way does not help.

Ask a third reviewer to step in if you can't agree

If you aren't sure if the code is correct, or you just can't agree on some aspect of the code, bring in someone else to settle things.

Provide feedback often

Without your approval, the code can't be released to the master branch or be available for production. As a code reviewer, your most important task is to shepherd this code into production in the most efficient way possible.

Too often, I'll push code for review, wait a few hours to have someone look at it, spend a few minutes addressing the comments, push some more code, wait until the next day for more comments, spend a few minutes fixing, wait a few hours, and repeat... The problem is that feedback cycle time is too long.

Once you review, you should quickly follow up with clarifications, do a second review if necessary, and generally be available to provide extra feedback. It's also important to be complete and precise with your reviews because it's just wasting time forcing the commit owner wait for feedback. This means that doing all your code reviewing once a day is not sufficient. Your team will be much more effective if there is a culture of providing code reviews quickly and often.

You want to minimize not only the number of feedback cycles but also the time it takes to complete each cycle.

As a rule of thumb, all devs on our team check the review queue roughly hourly. And if someone explicitly requests a review, someone is generally available to do it immediately.

People's emotions are tied up in their code

Even if you make a valid criticism, others may take it as an attack. As engineers, we try put facts above feelings. However, devs pour their heart into their code so there really is no separation between code quality and how they feel about it.

Never, ever make a personal comment about the commit owner or anyone else in the review. Never talk negatively about someone's skill. Never say that the code is bad or it sucks or even imply it in different words.

Instead, always treat the developer and their code with respect, even if the code needs improvement.

A while back, one of our front end developers once pushed some JavaScript changes along with related html/css. Not being an expert in JavaScript, that part of the code had errors. Errors similar to what he had made in the past. A code reviewer made a comment: This code is wrong, and it is just as wrong as last time.

It was a bad comment. It didn't tell the code author what he had done that was wrong, it didn't help him learn from his mistakes, and it made a sweeping assumption that had nothing to do with the code itself.

When you find a problem, be very specific about it and limit the criticism to that specific issue. We always assume that developer owner knows what he is doing and just needs a little help or support making the code perfect.

Also, it's important to make positive comments when the developer does something particularly clever or interesting. This is especially important for junior devs who may not yet have the confidence to know that they are actually making positive contributions to the team.

Differentiate between blocking and non-blocking comments

Some code review comments are blockers, meaning that the code should not be merged until they are addressed. Others are non-blockers and are only suggestions, questions, or observations that may need a response, and should not block the code going in as-is (though the code may change because of them).

For example, the following comments are probably blockers:

1. The extra database call makes the loop too slow
2. Please extract these lines to a separate function
3. etc

The commit cannot be merged the code is changed to address the comments.

And these are probably just suggestions or comments:

1. Do you think making this extra database call here will be too slow?
2. Consider extracting these lines to a separate function
3. etc.

A response is necessary for these comments, but they may not result in any code changes. Notice that the non-blocking comments are the same as the blocking ones, only more tentatively phrased.

Often, it's obvious what kind of comment it is, though sometimes it's not. This can be especially problematic for junior devs who don't yet have the confidence to push back on a suggestion even if it's not a good idea. It's extremely important to be clear as to what kind of comment you are making.

In some teams I've worked with, this was done explicitly. Prefixing a ! to a comment means that addressing it is a blocker. And prefixing it with a ? means it is just a non-blocking suggestion or question. For example:

1. ! This is an off-by-one error
2. ? Consider extracting these lines to a separate function

Other teams make the distinction implicitly through the phrasing of the comments, which can sometimes lead to miscommunication, especially if you have team members from different cultural backgrounds. In this case, there should be some explicit agreement that non-blocking comments are only suggestions, and will be addressed at the commit owner's discretion.

The following words imply blocking:

• error
• must
• important
• bug

And these imply non-blocking:

• suggestion
• maybe
• perhaps
• consider

In order to be as clear as possible, try to avoid using both blocking and non-blocking words in a single review comment. (eg- avoid these kinds of comments: Consider the suggestion that fixing this bug is important...)

Always provide a suggestion on how to improve

Be clear and precise as what needs to be fixed in order for the code review to be approved. When you see an issue with someone's code, first state clearly what the issue is, and then provide a suggestion or two on how it can be improved. For example, if I saw code like this:

function calculateP(q, b, t) {
 return q * b * t;
}

I'd say something like this: 

These variable names are difficult to understand, try renaming them to something that more closely reflects what they mean. What about calculatePrice, quantity, basePrice, and tax?

Even if your suggestions are not particularly good ones it shows that you've given thought about the problem and avoids giving a drive by review.

Avoid drive-by reviews

Sometimes a code reviewer (often a manager or a senior member of a different team) will (metaphorically) drive-by, drop a few blocking comments on a review, give some strongly worded feedback, and leave before the dust settles. Often when this happens, the comments will be lacking context, or missing key points even if the intentions were good. When I've seen this happen, there have been comments like this will never work, or this doesn't fit in with our architecture and needs to be changed.

There's nothing wrong with reviewing code in a project you're not familiar with. Actually, this can be a great way of getting a broader understanding of the larger codebase and disseminating knowledge across teams. But if you do so, you need to do it with humility, and avoid making blocking comments unless you are absolutely certain about them. Also, you need to understand that by reviewing a piece of code, you are taking partial responsibility for shepherding the code to production.

If you find that drive-by reviews are a recurring problem in your company, this indicates a larger lack of communication that can't be solved only through better code reviews.

Conclusion

You cannot be a good code reviewer unless you understand both the technical side and the human side of reviewing. Throughout this article, I've been focusing mostly on humans since that is often the hardest for engineers to teach and the easiest to overlook. You can't go wrong if you treat the code author with respect and you are explicit and diligent with your comments.

At Ganchrow Scientific, we use Gerrit Code Review. It's a great tool that enforces a structure for your reviewing and encourages best practices. I'll talk about it in a future article.

Happy reviewing!