Nitpicks That Waste Review Time

A code review that runs forty minutes over its slot usually has a body in the room. The pull request is 60 lines. Two of those lines change behavior. The other 58 rename a variable, reorder imports, and reformat a conditional, and every one of them has drawn a comment: the new variable name could be shorter, the imports should be alphabetized, the braces should sit on the same line as the conditional. By the time the actual logic gets discussed, four people are tired, the author is defensive, and the one real concern gets a perfunctory nod because nobody has the energy left to argue.

This happens in nearly every engineering organization that has been running long enough to accumulate opinions. Review time is a finite resource, and a startling amount of it gets burned on changes that do not alter what the code does. Not style, mind, which is a legitimate thing to have opinions about. The problem is the class of comments that feel like review but accomplish nothing except making the reviewer feel productive.

The rename that improves nothing

A reviewer sees data and writes "could be more descriptive?" A reviewer sees temp and suggests intermediateResult. The author sighs, accepts the suggestion, and the PR grows by eight lines of diff that touch nothing but identifiers.

The rename comment is the purest form of wasted review time because it almost never affects correctness and rarely affects comprehension. If data is obviously the payload from the request above it and the function is twelve lines long, a longer name buys nothing. The reviewer who cannot follow the code because a variable is called x has a different problem, and fixing it means reading the surrounding context, not renaming the variable.

There is a real category of bad naming: a boolean called flag, a function called handle that does five unrelated things, a variable that means the opposite of what it says. Those are worth a comment. The comment that changes userId to theUserId is a tax on the author's afternoon.

Whitespace arguments dressed up as standards

Every team has a formatter. Every team also has at least one reviewer who ignores the formatter and comments on the diff anyway. A blank line added between two related statements draws "nit: extra line." A line break in a chained method call draws "this could be one line." A comment gets reflowed to a different column width.

These comments are not about standards. They are about the reviewer's personal sense of how code should look, applied inconsistently across a codebase that already contains three different styles because nobody enforced anything for the first two years. The formatter exists precisely so that no human has to spend attention on this. When a reviewer overrides it, they are spending the team's collective attention on a preference that has no downstream consequence.

The test for a whitespace comment: would fixing it change anything about how the code runs, how it reads to someone new, or how easy it is to maintain? If all three answers are no, the comment costs more than it saves.

The conditional that could be a ternary, allegedly

An if-else that assigns one of two values draws a comment: "could use a ternary here." The ternary is then written, the reviewer approves, and the author spends four minutes on a change that will be reverted in six months by someone who finds the ternary harder to read with the line wrapped across three lines.

The same species includes "this could be a switch," "this loop could be a map," and "this could be extracted into a helper function" when the helper would have exactly one caller and four lines of body. Each of these suggestions is occasionally correct. Each of them is frequently made by a reviewer who is pattern-matching against a mental catalog of idioms rather than thinking about the specific code in front of them.

Idiom suggestions earn their place when they reduce the reader's cognitive load. A map replacing a five-line loop with a mutable accumulator is a real improvement. A map replacing a two-line loop is a lateral move that costs review time and produces a comment thread.

Comment typos and the reviewer who cannot let them go

A docstring says "recieve" instead of "receive." The reviewer comments. The author fixes it. The reviewer approves. Total elapsed time: the author's context switch plus the reviewer's second pass, which is long enough that the reviewer reads the whole diff again to re-establish what it does.

Comment typos are the cheapest possible review comment to make and the most expensive to route through a full review cycle, because they force a re-read of everything else. A reviewer who genuinely cares about spelling can fix the typo themselves in a follow-up commit, or note it and move on. Making the author context-switch back into a PR for a single character is a use of review time that would embarrass a cost accountant.

The same goes for punctuation in commit messages, trailing whitespace that the formatter missed, and the comment that says "nit:" about something the reviewer could have left alone for a year without anyone noticing.

Asking for tests that the change does not need

An author opens a PR that fixes a bug in a utility function. The reviewer comments: "Can we add a test for this?" The function is a one-line string transform used in three places, the bug was an off-by-one, and the test would assert the exact string output. The author writes the test, the reviewer approves, and the test now lives in the suite asserting that formatDate returns "2024-01-15", which is a test that fails only if someone changes the date format, which is not the bug the fix addressed.

Demanding tests is a reflex. It reads as rigor. But a low-value test has a cost: it runs on every CI pass, it accumulates as maintenance debt, and it teaches the team that tests are things written to satisfy reviewers rather than to catch regressions. The reviewer who asks for a test should be able to say what failure it would catch. If the answer is "nothing specific," the comment is theater.

The "while you're in here" scope creep

A PR fixes a null check. A reviewer notices an adjacent function with a similar null check that could also be fixed. The comment: "While you're in here, could you also fix this other one?" The author does, and now the PR touches two functions, the diff is harder to review, the commit message is a lie, and the second fix, which was never tested, ships alongside the first.

Scope creep is the most expensive nitpick because it multiplies. One "while you're in here" becomes three, the PR balloons, and the review that started as a fifteen-minute check becomes a thirty-minute re-read of code that was never supposed to be in the PR. The author's original fix, which was correct and small, is now buried in a diff nobody can evaluate confidently.

Adjacent improvements belong in a separate PR. The reviewer who spots them should file a ticket or open a follow-up, not attach them to someone else's change. The cost of a separate PR is a second review cycle. The cost of bundling is a review cycle that is no longer about the original change at all.

What the good reviews spend their comments on

A review that finds one real problem is worth more than a review that finds forty cosmetic ones. The real problems look like this: a function called with the wrong argument order, a conditional that inverts the original logic, a database query inside a loop, an error path that swallows an exception, a public API change that will break a downstream consumer. These are the comments that justify the review process existing.

Everything else competes for the same attention budget. A reviewer who spends forty minutes on variable names has forty fewer minutes for the logic. The author who spends an afternoon renaming variables has an afternoon less to spend on the actual feature. Across a team of ten, the arithmetic gets ugly fast.

The fix is not to stop caring about code quality. It is to separate the two categories cleanly. Correctness, security, data integrity, and public API compatibility are review concerns. Style, naming, formatting, and idiom preference belong to a linter, a formatter, or a separate cleanup pass, and if the team has not automated them, that is a tooling problem masquerading as a review problem. Asking reviewers to be the linter is how review time gets wasted.

The one-line PR with eleven comments

There is a particular artifact that shows up in team retrospectives: a pull request that changed a single line and accumulated eleven comments. The line was something like moving a null check from one side of an assignment to the other. The eleven comments were about the variable name, the surrounding whitespace, the comment above the function, the test that should have been added, and a suggestion to extract the whole function into a helper.

The change shipped. The bug was fixed. The review took three days. At the retro, someone asks why the PR took three days, and the answer is that nobody in the thread noticed they were arguing about everything except the line that mattered. That thread is the artifact worth keeping, pinned where the team can see it, as a reminder of what a review is for.