Miserable code reviews, and how to fix them

Code review is a crucial part of any healthy software company’s delivery process. Reviewing proposed changes reduces the risk of shipping bugs. It also gives both the reviewee and reviewer a chance to grow—the reviewee gets feedback that improves their code, and the reviewer gets to look critically at code they might not write themselves.

Sometimes, code review is infuriating.

It’s not the idea of review itself that causes pain, but the way it’s practiced. Review can often feel like more of a roadblock and less of a safeguard, and this can wear away at a team. I've compiled a list of common issues I've seen with code review, and how I've addressed each issue in my personal practice.

1. Style comments

This one's a classic. “Tabs vs. spaces” debates, inconsistent formatting, or personal style preferences. These discussions gum up review and don’t improve code quality.

Simply put, personal preference has no place in code review. Design your CI system such that a change cannot merge unless it conforms to an agreed-upon style, created and documented by your team in advance.

Pick a style, document it, and enforce it automatically. If you have a style preference that cannot be represented by a code analysis rule, document it ahead of time. Any style comments should include a link to where the developer could have seen the rule for themselves. If no such links exist, delete the style comment you were going to write, write the rule, and fix it yourself in a follow-up change.

2. Unclear expectations

I've reviewed an unfortunate number of changes that could not have possibly been tested (manually or otherwise) by the author. The changes might contain a bug so face-punching that any developer who has run the code would have immediately seen it, or the changes may not build at all, as evidenced by a wall of red "X"s in Github Actions.

I'd become incredibly frustrated when this happened. It got so bad that I would refuse to review code written by developers who had a history of PR-ing untested changes. This hurt both the quality of the codebase (untested changes were merged anyway, often without review) and my professional relationship with my peers.

When I eventually confronted a developer about it, I learned that they had been using PRs as a sounding board, looking for feedback on rough drafts. This differed from how I used PRs (as a way to propose completed + polished changes). Since our team never formally communicated how we do PRs/code reviews, the developer's approach to the process wasn't unreasonable, and I felt silly for my initial reaction.

Write a blurb wherever your team keeps their documentation around code review expectations. Make it clear what's expected of reviewees and reviewers, and make those expectations highly visible. Does code require approval, and from whom? Should changes include automated tests? Should reviewers manually test changes? Should reviewers manually test changes? Answer all questions that, if unanswered, introduce ambiguity into the process.

3. Unclear actionability

Not every nitpick needs addressed in code review, especially if a change is time-sensitive. Not every suggestion needs to be implemented. Not every thought needs a response. But how does a reviewee know which comments are important? What's blocking, and what can be addressed in a follow-up?

Last year, a coworker introduced me to Conventional Comments. The concept is simple: every review comment gets a label. The label helps communicate intent and tone, signaling what's expected of the reviewee in response.

For example, a nit: comment is nice to address, but is never blocking. An issue: comment represents a problem, and it should block the changes. A thought: might spark discussion, but does not warrant a response, in code or otherwise, from the reviewee. Conventional Comments help reviewees determine what's actionable, and what they can safely ignore if in a pinch.

Pair Conventional Comments with a very clear approval or rejection of the PR. The only time a review should be completed without an explicit approval/rejection should be when there is an outstanding question that, when answered, could impact the correctness of a change. If a PR is approved, it should be able to merge without any further action by the developer. If a PR is rejected, it should be clear exactly what changes are required to gain approval. Aim to always leave developers with a "next step" when reviewing code.

Code review becomes a tool for shipping better code and helping the team grow. If your process feels broken, aim to improve it.