Improve your code reviews

Everybody does code reviews nowadays (I hope!). Research shows that it increases quality... blah, blah, blah.

But, how many times did you think about how much does it cost? Because you know ... Code Review isn't free. Code Review takes time. A developer publishes a PR, and then... what? Is there anyone readily available to review it? Other devs are busy doing their own stuff. Should the developer interrupt someone to get a fast review? Or just context switch to something else for now? I'm sure the research was not investigating this part.

Before most readers close this page enraged, shouting “Obviously, Code Review is good, you fool! It's totally worth it!” Yes, Code Review is worth it! I didn't say it isn't. I only said that it does have productivity cost and you should not ignore it. Please, while you are aware of the benefits you're getting, also acknowledge the costs you made to gain them.

I'm not just trying to be controversial and/or annoying. My point is: if you want to be good at something, you have to be mindful of both of them: benefits and costs, to maximize the former, while minimizing the later! Most of my guidelines below are going to be about minimizing the cost of Code Reviews.

It seems to me that there's a tendency to focus on the benefits, ignore the costs, and many people think that code review is all about spotting mistakes and problems. And they think they do a great job because they can always point out... some more mistakes. And they completely ignore the cost of their reviews: time, motivation and productivity wasted: both theirs and their peers.

I thought a lot about it, and in my opinion, The Golden Rule of Code Review is be a helping hand, not a roadblock. Your goal as a code reviewer is not what it might seem: you're here to make your team do a better job, faster, not slower than they would without your code review. By pointing out problems that would waste time in the future, giving ideas that might help, and mutually sharing knowledge to get even more productive.

Be readily available. Always try to promptly review your peer's changes. You don't have to interrupt what you've been doing immediately, but if you can, by all means – review changes first, to unblock people waiting on you.

Eliminate needless round trips. What really annoys me is when I submit a PR, get a review, address all the problems and suggestions, submit it again for review... just to get the next batch of unrelated requests from the same reviewer. It feels like the reviewer really just wants me to go away, and they are just looking for an excuse to get me of their back. Even though I know it was probably never their intention, that just the impression it gives. As a reviewer – always try to make sure the change author will not have to do back and forth needlessly. Start with doing a complete review all at once and being mindful about author's time and effort.

Be cooperative and interactive. Make it clear that you're here to help get things done. If the code author can promptly address the issues/answer questions, you can follow-up immediately. Try to make the code review more like a quick chat, less like a slow bureaucratic process where everyone has to go past you.

Don't point out the problems. Point out the solutions. Unless the issue is trivially fixed, it's not enough to just point out there's a problem. You should always offer the best solutions you know of.

Focus on what's important. As a principle, I don't bother pointing out “nits”: spelling mistakes, code style problems, irrelevant lines, etc. They sometimes do matter (eg. writing public-facing documentation), but in a lot of SWE daily work they are totally irrelevant. If someone makes a lot of mistakes, or formatting is totally messed up, I'll just let them know and point them to some tools that should address that. What I focus on is the big picture – is this the right approach, is the architecture good, are there any high-level issues. Then I slowly move towards details.

Define the bar properly. Someone submits a change “Change this O(n^3) into O(n)” and in response gets a request for changes with a response “this could be O(log n)”, ignoring that it will take a half a day of work to implement O(log n). The original author is pulled into different work, forgets about this PR, the code keep being O(n^3). I tend to use The Boy Scouts Rule as my default bar – if the change makes things better, then it can land immediately if the author so desires. If the author wants and can, they can follow up with any further improvements.

Just be helpful. Put yourself in the author's position and genuinely try to help them out. Think about what are their motivations, goal, expertise, experience level, etc.

Bonus: Use Code Reviews as an instrument for knowledge sharing. I find Code Reviews a good place to... chat. Using a problem and code at hand, it is possible to much more naturally discuss opinions, experiences, and make relevant remarks, and just understand different points of view. There's always more to learn, and it goes both ways. Obviously... if both of you have a moment to spare.