Often times, a code review can leave both the author and the reviewer with a great deal of angst and trepidation. But it doesn’t have to be that way.
In this article, I’ll share some of the things that I’ve learned over many years reviewing code as both a developer and manager. Although I’ll touch on some things that affect the author of the code, I’m writing mainly from a code reviewer’s perspective.
But before I begin, let me tell you a story…
As a development manager at one of my previous companies, I regularly performed code reviews on my developers’ pull requests. I had one developer in particular who just couldn’t take criticism. Whenever I made any comments on his pull requests, he got all bent out of shape and accused me of picking on him! He just couldn’t take constructive criticism …or so I thought.
It turns out that I wasn’t very good at giving constructive criticism. I thought all I had to do was point out the issues in a cold, to-the-point way. That works fine for some people, but not for others.
Something had to be done, and it got me thinking about how to perform an effective code review.
Critique, don’t criticize
All too often I hear developers say, “I don’t want to be the one who criticizes another person’s code.” The word “criticize” comes from the same root word as “critical” and “critique.” To critique something is not a bad thing. So the next time you do a code review, try not to think of it as criticizing; think of it as an honest and helpful critique of someone’s work.
an analysis or assessment of something, typically art, literature, or music
Choose your words wisely. Use suggestive, collaborative and encouraging words instead of demanding or barking.
Instead of commenting:
“Change this to use a temporary variable.”
You might want to say instead:
“This might be more readable if we changed this to use a temporary variable.”
See how this changed the tone of the discussion? It brings the author into the conversation. Who knows, the simple gesture of asking may even spark a better alternative solution.
It’s everyone’s code
When you work on a team, the responsibility of gating dirty code from getting into the code repository is shared by everyone. Some of the most valuable advice I give developers is to treat the source code not as your code, or their code, but as our code.
At some point down the road, you may find yourself working on code that another developer is checking in today. It’s not only your right, but your obligation, to make sure that the code is of the highest quality.
Let your tools be the bad guy
The last thing that you want to do as a code reviewer is to comment that the author forgot a semi-colon, or a space before a parenthesis. It is a waste of your time and it makes you the petty reviewer. You have helpful, substantive, and encouraging things to say in your review. Leave the rest to automated services.
The modern software development world has many tools to automate what used to be a manual process of reviewing code. There are a number of commercial and free services (like Travis CI and CircleCI) that can perform unit tests before a human ever needs to get involved.
If your organization has rules about code style, let an automated linter do the complaining.
Most of these services are free for personal or open source use. Use them!
Don’t be afraid to be wrong
If you see something in the code that doesn’t look right, but you’re not quite sure, go ahead and ask. It’s OK to be wrong. Just leave a comment and ask.
“Are you sure that this shouldn’t return a
It causes the author to recheck the work. They are, after all, the subject matter expert. You may get a reply like this:
“Yes, I’m sure.
LOADEDis the correct return value here.”
And that’s fine. In fact, you just learned something. But who knows – you may get a reply such as:
“You’re right. This should return
PENDING. Good catch!”
Congratulations. You just caught a possible production bug.
Never just rubber-stamp it!
Here’s a scenario for you.
You’ve been asked to do a code review for Allison, a highly respected senior developer on the team. You go to this person all the time for help and advice. “Her code can’t possibly be wrong,” you say to yourself. “I should just approve it.”
Here’s another one.
James did you a favor last week. Now he’s under pressure to get his latest feature out the door, so he comes to you and says: “This pull request is pretty minor. I refactored a few things and added a few files. No big deal. Just approve it for me, will ya?”
The answer to both situations above is: No! Neither the skill level of the author nor the urgency of the situation matter. You should approach each code review with the same level of commitment.
Give helpful suggestions
If you are suggesting a change, please don’t just describe your issues, or say something like, “This is all wrong.” Try to give practical code samples or links to supporting documentation where applicable.
This is a partnership. You should give the author every opportunity to succeed. Don’t make them guess what you are thinking.
You should treat the developer with as much respect as you do the code, and vice versa. Writing code in an organization is a team sport.
Remember… There is no “I” in “CODE.”