5 Tips for More Helpful Code Reviews

Submitting code for review can be daunting. After all, it is exposing our work to critique, and that can be terrifying regardless of the industry. But code review can also be a great tool for learning and improvement. As reviewers, we can make all the difference. Here are five things I do when reviewing code to try to make it as helpful as possible for the author.

Rewrite comments for empathy and clarity

In code reviews, a comment is interpreted by how the author reads it. Therefore, as a reviewer, you should consider the potential impact of your words, not just their original intent. Otherwise, you may be surprised to find that a comment you meant to be helpful was merely hurtful.

That’s why rewriting comments until they clearly convey your meaning with empathy is very important. Sometimes we act as though writing comments is like drawing with a permanent marker: what goes on the first time is the final product. But the opposite is true — most good comments are rewritten comments.

This module name is obviously confusing What do you think about clarifying this module name?

What do you think about clarifying the module name UserCreator? I found it a bit confusing since we’re not actually creating a user. We’re only inviting them. What do you think about something like UserInvitation or UserInvite?

After writing a comment, read it out loud to yourself. Is there possible ambiguity? Can you be more specific, even if it’s more verbose? Now try to read it from the perspective of the author. Are there sentences that sound angry, upset, or belittling?

Look at the example above. The first comment is potentially ambiguous (what module is the reviewer talking about? Why is it confusing?) and belittling (“obviously confusing”). The second comment is no longer belittling, but it’s still ambiguous (what’s unclear about the current name?) The third comment is clear (using a specific module name and stating why the reviewer finds it confusing), it is not belittling (not “obviously confusing” but “I found it … confusing”), and it even anticipates that naming is difficult, so it makes a few suggestions to help.

Include code samples

If you’re suggesting changes, don’t speak in extremely vague terms and assume your entire team understands what you mean. Instead, try to include code samples of what you’re recommending.

Here’s an example of a comment I might write:

What do you think about extracting the creation of the user so the whole #create method is at the same level of abstraction? Maybe something like this?

# instead of this
def create
  # this is at one level of abstraction
  email = params[:email]
  password = params[:password]
  user = User.create(email, password)

  # this is at a different level of abstraction
  send_email_to_user(user)
  notify_admin(user)
end

# we could have this
def create
  # this is now all at the same level of abstraction
  user = create_user
  send_email_to_user(user)
  notify_admin(user)
end

private

def create_user
  email = params[:email]
  password = params[:password]
  User.create(email, password)
end

Of course, there are times when this may be unnecessary. For example, when reviewing code from an author I know is familiar with what it means to write methods at the same level of abstraction, I might simply ask the first question above without including the code sample.

Whether or not the suggested change would benefit from a code sample will always be, in the end, a matter of judgment. Nevertheless, consider that code reviews are public conversations from which the whole team can benefit. And even experienced developers gain from ideas on how to implement a suggested change. So when in doubt, I recommend including the code sample.

Find good things to say!

This one is very important. It’s easy to list all the things you think need changing in the pull-request but gloss over all the good things present. If you see something good, say something good! It’s refreshing to receive positive feedback. I find that even simple things like these can go a long way:

• “I love this method extraction”
• “These tests look great! 🎉”
• “Nice catch on this poorly named method! Thanks for changing it”

Include links, links, and more links

If you reference a function, don’t assume the author of the code knows exactly what function you’re talking about. Include a link to its documentation.

What do you think about using `map` map instead of each here since we want the return value?

If you’re referencing a previous commit or pull-request, include a link to that.

In a previous commit 172a0f, we made it easier to copy the dependency when installing.

And if you’re talking about a particular concept with which the author may not be familiar, try to include a link to a blog post that elaborates more on the topic. Blog posts can be powerful tools for learning.

What do you think about writing this acceptance test at the same level of abstraction same level of abstraction?

Whatever you do, make it easy for the author to get to the things you reference. Ideally they’re only one click away.

(Breaking down the fourth wall: did you land here because someone linked this blog post in code review?)

Offer to chat and pair

Finally, I like to (a) offer to talk in person (or video call) to explain anything that is unclear about my comments, and (b) volunteer to pair on any of the changes I suggested. In addition to being a nice gesture, it opens the door for further communication and collaboration. Just be sure to follow through when someone takes you up on them.

What next?

After writing this blog post, my co-worker (the great) Mike Burns shared this Reviewing Pull Requests blog post with me. I found it very enjoyable, so if you liked this blog post, you might enjoy it too!

And if you’re interested in learning more about improving both sides of the code review process, take a look at this talk on Implementing a Strong Code-Review Culture.