How to Ruin Code Review

Code review is a powerful tool for ensuring quality, preempting bugs, and improving shared understanding. Spend enough time at it, and you’re guaranteed some wonderful moments: the satisfaction of a shared revelation, say, or the collective sigh of relief at a breaking change stopped short of production. You’re also assured at least a few stinkers.

At its best, peer review is a time for knowledge transfer, alignment, collaborative improvement, and team growth–but this story isn’t about that.

I promised a guide to crippling peer review, so here it is: a collection of simple strategies for inflicting misery on reviewer and author alike.

As the author

It’s your move! As you take one last look over the hard work you’re about to publish, consider the agony you’re about to inflict.

  1. Submit a monster. Besides the sheer time and energy it takes to comb through a large diff, large changes–or changes that attempt to tackle multiple issues in a single review–leave plenty of corners for defects to hide in. With the reviewer struggling to understand the scope of the change, whatever commentary they provide is likely to be as unfocused as the code it addresses–bad news for constructive code review, and bad news for quality overall.

  2. Don’t explain why. Whether delivering value to a customer, fixing a bug, or just scrubbing old code, every change is up to something. Hiding the raison d’être of any but the most trivial change will beguile your reviewer and leave them struggling once again to understand what they are supposed to be reviewing.

  3. Change things for change’s sake. Code is occasionally written without a reason to be. Submitting it for peer review–or including unused code in a broader, useful change–is a great way to clog the queue and distract from work that is actually adding value.

  4. Take it personally. Review is about the feature, the team, and the longterm sustainability of the codebase. Mistaking constructive criticism for a personal attack is a surefire way to start an argument, breed ill-will, and drain the value out of an otherwise productive review.

  5. Submit less than your best. Peer review isn’t development. When incomplete (or broken) code is submitted for review, it invariably ends up rewritten and resubmitted for a fresh round. Skipping local validation–both form and function–of a change’s behavior will frustrate the reviewer and waste everyone’s time.

As a reviewer

You have a few tricks of your own! Not to be outdone, you can easily sap your critique of any effectiveness it might otherwise have had. Just:

  1. Ignore the context. Dive into review before determining what you’re reviewing and the issue that it’s designed to resolve. This won’t just lead to confusing, off-topic commentary as you talk past the problem at hand–it will also ruin the author’s own thoughtful attempts to describe problem and solution in the structure and specs you are reviewing.

  2. Rewrite everything. It’s better to catch architectural flaws in review than an overhaul months down the line, but code review is rarely the right forum for tackling issues far upstream. Assuming the development process is in working order, the author has typically invested more research and consideration in their work than the reviewer. While the back-and-forth of reinventing their work may be good for education, it’s sure to halt the development process and distract from the review at hand.

  3. Pick nits. The sweet spot for code review lies somewhere between high-level design decisions and the sort of minutiae that are easily enforced through static analysis. Calling out formatting changes, entity names, and inline comments that don’t impact function can help delay the merge of otherwise useful code.

  4. Attack the author, not the code. Personal attacks will put the author on the defensive. Any energy invested in self-defense is energy unavailable for developing great software–and that’s not to mention the ill-will created ahead of the next review.

  5. Bring the dogma. Time marches. Context changes. Guidelines that were relevant one day may not matter the next–but adhering to them anyway can surely slow the pace of progress. Every change begs the question, “what’s changed around it?” Convention exists for a reason, but applying it inflexibly is both an easy way to resist change and a guarantee of eventual stagnation.

On the other hand

And there you have it–code review, without the good parts. Spend enough time at it, and you’ve seen it before. Pedantry, inflexibility, ignorance, and every excuse imaginable to keep a change from reaching production.

On the whole, I have yet to encounter a peer review process doing more harm than good. It’s an efficient, valuable tool for developing a team and its code.

Let’s end on a positive note. Inverting the “do-nots”, we can arrive at an equally relevant guide for encouraging expedient, positive code reviews.

As an author:

  1. Focus changes (one issue per review)
  2. Include context (reference issue numbers, write effective commit messages, add commentary for your reviewer)
  3. Avoid pointless or self-serving changes
  4. Use constructive criticism to grow
  5. Validate your own work (skim the code; verify its behavior)

And as a reviewer:

  1. Understand the context (supporting issues, previous code)
  2. Give the benefit of the doubt
  3. Don’t pick nits (computers are good at this)
  4. Review the code, not the author
  5. Be flexible

That’s it for now! What elements do you see in great code review (or in the reviews that you’d rather forget)? Share them on Twitter.

Further Reading

Featured