Github has added some new features to pull requests that make for a richer code review cycle. To commemorate this, I thought I’d share some words about code review–not the Github stuff specifically (I’m still exploring!) but the process in general.
Code review is a cornerstone of good team-based software development. It isn’t the same as pair-programming, and it isn’t a replacement for it. By the same token, pair programming is not a replacement for code review. The two practices work in tandem–and, as I have suggested elsewhere, code review can also make solo coding a viable part of the development process. I consider it absolutely essential.
In this article I’m going to go over three guidelines for successful code reviewing:
- Do your code reviewing alone
- Be fair and flexible about style
- Treat unclarity as a red flag
I’ll say something about each of these guidelines in turn.
Do your code reviewing alone
If you are pairing on code review, your pair is either (a) one of the people who wrote the code, or (b) someone else who didn’t write the code. Both options are problematic.
Reviewing code with someone who originally worked on the code inevitably means that you’re seeing the code through their eyes. You’re really asking for a guided tour, not doing a review. In review, code should speak for itself. You don’t want someone there explaining it to you. That’s not representative of what it would be like to work on it yourself at a later date, or for someone new to the team to pick up a story that required them to work on this code.
Moreover, you might–without meaning to–put your pair on the defensive. “Why did you do this?” you might ask, pointing to some line or lines of code. The person who wrote or co-wrote those lines then has to mount a defense of them, and that can be frustrating as well as unproductive. Lines of code in isolation sometimes don’t explain themselves; you, as the reviewer, might have to look at the whole class or file to see the rationale for the code’s structure or naming conventions. But quizzing the coders on why they did something is not the same, cognitively, as looking through the code yourself and letting the code speak (or fail to speak) to you itself.
Doing code review in a pair where neither of you worked on the original code is also not advisable. Each of you will pick up on different things, and pointing those things out–not to mention scrolling up and down and flipping among files out of sync with each other–is simply not the same process or experience as examining the code yourself. Your different paths through both seeing and understanding the code will cause interference patterns and almost certainly make the task less efficient and less effective.
Now this doesn’t mean that you should never sit down with someone else and look at code! I’m talking specifically about code review: someone has just committed some code, and it needs that next pair of eyes. You may have occasion to discuss code you didn’t write with someone who did write it, and you may have occasion to discuss code with someone else when neither of you wrote it. (Think of a pair picking up a ticket on some code by people who have left the team.) What I’m saying is not that it’s bad for these things to happen. What I’m saying is that they are not code review.
Be fair and flexible about style
Your team should have a working agreement about code style. It may be a blanket agreement (“When we write Python, it has to pass Pylint”). Or it may be a very specific agreement (“No empty parentheses in Ruby method definitions”). How you determine your team or “house” style is a matter for the team and the organization.
When you do code review, you should not hesitate to point out violations of team style agreements. However, if the code you’re reviewing does not violate team style–but happens not to be in the style you yourself would have used–leave it alone. Code review isn’t about making everything look as if you had written it.
If you spot something stylistic that you think no one on the team should be doing, make a note of it and bring it up later as a possible addendum to your team’s style agreement. But don’t flag it in code review if it isn’t already part of that agreement. Code review is not the place for stylistic ping-pong matches.
Treat unclarity as a red flag
Perhaps the main thing you should be looking for during code review is code that is not clear. Unclarity can take many forms; but whatever form it takes, it should be treated as a show-stopper. Unclear code means someone is going to get confused at some point down the road–possibly including the people who wrote the code in the first place. (We’ve all had that happen!) You should immediately write a comment when you come across something unclear. In your comment you should include some analysis as to why you think it’s unclear: a method that’s doing too much and is hard to follow, confusing variable naming, and so forth.
This guideline is actually not only important in its own right, but also figures as another reason not to do code review alone. If you do code review in pairs, it’s too easy to explain the code to each other as you go. You don’t get enough of a chance to determine whether or not the code per se is clear.
Don’t be afraid of insulting someone’s code by saying it’s unclear. By the same token, if someone reviews your code and says they didn’t understand or couldn’t follow something–whether because of logic or naming or anything else–don’t defend or explain your code. It’s likely that you could explain the unclarity away. But you shouldn’t have to, and in the future when other people look at your code, you won’t have the opportunity to. Consider the finding of unclarity to be a reality check, and a very compelling indication that you need to change the code.
Happy reviewing!