I think that is the most useful thing that a community of programmers can do for each other—spend time on a regular basis reading each other’s code. –Douglas Crockford1
We agree with Crockford on this one–regular code reviews are a key component of the developer culture at UserTesting. Getting this practice right is especially important to us because it sits at the intersection of several of our core company values–Get Better, Keep it Simple, and Be Kind.
Developing and maintaining a healthy code review culture is not without challenges. Throughout our careers, we’ve all seen it go wrong in various ways. The good news is that there are a number of strategies for overcoming these common stumbling blocks.
1. Getting Personal
A common issue with code reviews is that people can sometimes feel personally offended by some of the comments. This isn’t too surprising: code reviews are an opportunity to provide feedback, which often comes in the form of criticism, and criticism is a tricky thing. It’s easy to give criticism in a careless way, and it’s easy to interpret criticism as a personal attack.
Constructive criticism is essential to growth, but in order to be effective, both the giver and receiver need to be thoughtful about their communication.
The Prime Directive
A good first step is for everyone on the team to understand and agree to the Prime Directive. It’s most often used in retrospectives, but why not apply it to any situation where you’re giving direct feedback? Here it is:
Regardless of what we discover, we understand and truly believe that everyone did the best job they could, given what they knew at the time, their skills and abilities, the resources available, and the situation at hand.” –Norm Kerth2
Setting up this attitude lets the reviewer know the job is not to judge the individual who wrote the code, but rather to make suggestions for improving the code itself. It also lets the code author know that comments need not be interpreted as personal judgments, but rather as opportunities for growth.
It’s also important for commenters to be mindful of tone. For example, consider the following comment:
This is the wrong way to structure the test. There are too many expectations in one example.
While not directly personal, this type of comment also isn’t likely to be well received.
As Hendrie Weisinger explains in The Power of Positive Criticism, this kind of right/wrong distinction threatens an individual’s self-esteem, reducing the likelihood the recipient will be open to the feedback.3
The comment above could be rephrased as,
I think it would help with readability to split these expectations into separate examples. Here’s a reference: http://betterspecs.org/#single.
Another easy way to protect someone’s self-esteem is to ask questions rather than stating opinions. For example, suppose you’re reviewing code and you think,
These 3 lines really belong in a separate method.
Instead of writing that, you could write,
What do you think about extracting these lines into another method to isolate the calculation logic?
It’s a small tweak, but this style of feedback communicates respect to the recipient and provides the space for a productive discussion.
Note that simply phrasing your comment as a question isn’t enough. Take this example:
Why didn’t you just use a factory?
As Stacy Kvernmo of Collegis Education has pointed out, using the word “just” implies that the coder missed something obvious4. It’s easy to transform this comment into something more respectful:
Did you consider using a factory here?
It’s worth the effort to protect your teammates’ self-esteem not only out of kindness, but also in order to strengthen your working relationships. Weisinger describes the process: “Since the recipient’s ego has been left intact, or even enhanced, her perception of you is likely to improve to the point where she sees you as a credible source,” and then, “later criticisms are welcomed.”3
2. Wrong Time, Wrong Place
Sometimes code review comments get off track. That’s not to say people start discussing random topics, but more that they’re discussing the code at a level inappropriate to the code review.
For example, how many pull requests need comments debating the appropriate amount of whitespace? Code reviews with long debates about issues of style indicate that the team needs to establish some conventions and agree to stick with them.
Once you have a style guide in place, code review comments may still, of course, include discussions of style issues, but they can include a link to the style guide instead of stating an opinion and opening up a debate each time.
It’s even better when you can integrate an automatic style-checking tool like Hound into your workflow. Then you can minimize the human involvement in style guide enforcement.
This will save everyone time and allow for the discussion to focus on the more important and perhaps more subtle issues.
On the other end of the problem, if code review comments are too high-level, it might be a sign that a much-needed architecture discussion didn’t take place earlier in the process.
If there are any architectural problems with the code, the reviewer has a duty to call them out, of course. Final code review is just not the optimal place for a sweeping discussion of how to approach a problem. Ideally, these problems are identified in a whiteboarding session before a line of code is written.
Another common problem with code reviews can be that people don’t do them. Pull requests sit around for a while waiting for their comments. This can happen for a number of reasons. As Jamie Zawinski puts it,
…it’s just more fun to write your own code than to figure out someone else’s.1
Code reviews may not be inherently fun, but there are a few steps the code author can take to make the process a little more palatable.
I’ve certainly been guilty of asking people to review a pull request with thousands of lines of code. The problem is people can feel rightfully overwhelmed by such large changes. It’s really up to the code author to keep changes as small as possible.
Of course, occasions arise where you can’t avoid a big change. In those cases, it’s often worth scheduling an in-person code review meeting. It will likely be more productive than a long comment chain.
In fact, no matter what the size of your pull request, sometimes it just makes more sense to schedule a code review meeting. It may end up consuming more time, but it’s one way to help elicit the feedback you need.
That said, there are also ways to present your code changes within a code review tool that will help attract reluctant reviewers.
It’s often helpful for a pull request comment to include context and an explanation for the code changes. UserTesting’s Matt Snyder introduced a handy Why/What/How framework for describing code changes.
One of our front-end engineers, Emily Stewart, makes her pull requests more approachable by including handy screenshots of resulting user interface updates.
Since UserTesting is such a remote-friendly team, some team members even create screencasts to walk through their code changes, which is especially helpful.
Anything you can do to help potential reviewers understand your changes will help. At the very least, you can link to the relevant user story.
There is one caveat here, though, from Gerald Weinberg:
People who spend much time debugging other people’s programs soon learn not to listen to explanations before tackling the problem, for these explanations tend to put the listener on the same false track of assumptions…5
The key is to strike a balance. Reviewers can use the explanations to get their bearings, but may ultimately need to set them aside in order to evaluate the code.
The Good News
Once you get a solid process in place, code reviews become a great opportunity for learning, and the overall code quality improves. Continued success requires a team to remain committed to the process and educate new hires along the way, but enjoying a healthy code review culture is well worth the effort.