How to Be a Better Code Reviewer

A couple of weeks ago, I gave you a few hints on How to Be a Better Code Reviewee. This time, we’ll look at the other side of code reviews and examine some good practices for code reviewERs.

Don’t Block People

Code reviews, especially when held using online tools, can feel blocking for people as they might not want to start a new task when their previous one (or 7 previous ones as happened to me once) is still in review. It’s also desirable that the reviewEE has enough time to apply changes before the sprint ends. Therefore, get down to do the code review in a timely manner.

Take Your Time

This point may seem contrary to the first, but in reality, it’s not. Once you find the time and sit down to do the code review, don’t hurry. How do you want to evaluate if the solution is correct without its careful analysis or if the code is readable if you don’t read most of it? This might not apply if there are other forces in constraints in place. In such cases, use common sense to balance between speed and quality of the review.

Be Specific

Whenever you find something wrong with the code, try to be as specific as possible. Try to articulate what change exactly do you suggest. If necessary and possible, you can even provide short code samples. There’s a high chance that the person who submitted the code believes it’s good enough. It won’t help if you write comments like Wtf? or Correct it, please (I heard about such cases). I know that sometimes you see the code and it just doesn’t “feel right” and you don’t know why exactly. In such case, try talking to a rubber duck or the code reviewEE about the ways things could go wrong with the solution.

Read the Tests

It’s so, so obvious and yet people fail at this (including myself, sometimes). We should care for the test code the same way we care about the production code. There are no clean, robust and fast test suites without proper care applied to develop them. Another thing is, you want to make sure that all test cases are covered, do you? You can’t do that without actually reading the tests. And if you find some test case missing, there’s a high chance that you just found a potential production bug.

Don’t Make It Personal

However hard they wouldn’t try, people WILL be personal about the code they write. They will treat it as their lovely, little kid and react defensively when the critique is too harsh. Therefore, try to make the code review feel like final polishing of the code, instead of proving the other person stupid. Avoid any kind of personal comments. Make it all about the code. When the solution is so bad that it requires a complete overhaul, try to make them feel like it’s you have an alternative idea to their good one, not an alternative idea to their awful one.

Provide Arguments

“It’s not how I would have done it” is not an argument. Unless the change you’re suggesting is obvious, you should explain why do think the code should be changed and what would be the consequences of going either way. You’re not having any special rights just because you’re a reviewER. If you want the code to be changed, you need to think how to “sell” the idea to the other person. Sometimes you might not feel good enough to explain everything by yourself. In these cases, try to provide some resources that may justify your opinion (just not 800-page books please!).

Don’t Make Sure It’s Perfect, Make Sure It’s Good Enough

As the code gets cleaner, the time passes and you keep thinking about the reviewed solutions, you might keep getting new ideas how it could be improved. It’s normal among people so brilliant as programmers are. 🙂 However, there needs to be a stopping point at which the code is accepted and all other changes will be applied in subsequent stories. The business wants to make money and so the project has to move forward. Therefore, work on improving the code until it’s good enough to make money for a long time, not to print it and hang it on the wall.

Prefer Discussion over Online Comments

I know I’ve written that in the previous post about reviews, but it’s important and cannot be stressed enough. If you’re using an online tool for code reviews even though the team is co-located, try to limit the online comments to stuff that you both agree on. Once you recognize a disagreement, it’s better to take things offline. The kinds of problems that take hours and tons of comments can usually be resolved in a few minutes of productive talk. And you can drink some tea at the same time!

Let the Reviewee Know When You’re Done

Some online tools (like Stash) lack a way to let the reviewEE know that you’re finished with the review. In such case, it’s good to employ some informal way of letting the other person know that you’re done e.g. writing a comment I’m done! or just saying it out loud. It’s a little thing which might sound stupid to offline review practicioners, but I find it really helpful.

About the Author Grzegorz Ziemoński

King of Tidy Java, nerd that thinks about producing perfect software all the time and proud owner of 2 cats.

follow me on:
  • Mariusz Lisiecki

    Actually, Stash has the option to tell the reviewer you are done. You can either mark the Pull Request as Approved or Needs Work. The second option has been added later (until then we used to post comment “Review finished” – similarly to what you suggested).