Code review is teaching

Two comments I remember.

In 2015 somebody reviewed a form handler I’d written and left: “This works. Have a look at what happens if someone submits twice quickly, it’s a thing that bit us on the last project too and it took me ages to spot the first time.” I fixed it. I have thought about double submission on every form I have built since, which is five years, which is a decent return on one sentence.

And last year I left “why is this here?” on a junior’s pull request, on a line that was, in fact, fine. I meant it as a genuine question. They deleted the line.

It’s not actually very good at catching bugs

The standard justification for review is defect detection, and I’ve come to think that’s the weakest of its claims.

Reviewers mostly don’t run the code. They read a diff, out of context, without the state of the system in their head, having been interrupted to do it. The things that are findable that way are a subset: obvious logic slips, a missing null check, something that’ll fall over on empty input. Genuine bugs, the ones that cost you a weekend, come from interactions that a diff cannot show you.

And there’s the size effect, which is the bit nobody likes admitting. A 40-line change gets thoughtful comments. A 900-line change gets approved in four minutes with a thumbs up, because nobody can hold 900 lines and everybody knows the author has been on it for a week and is waiting.

A forty-line change gets thoughtful comments. A nine-hundred-line change gets approved in four minutes, and everybody involved knows exactly why.

So if bug-catching were the point, review would be a fairly expensive ritual with a poor hit rate, and I’d argue for spending the time on tests.

The reason to do it is that it’s the main way a team transfers how it thinks, and this year it’s very nearly the only way, because nobody sits next to anybody any more.

Anything a machine can say should be said by a machine

Before any of the human stuff: get the mechanical comments out of the process entirely.

Spacing, quote style, trailing commas, import order, line length. Every one of those comments costs a round trip, a notification, a context switch, and a very small amount of the author’s dignity, and none of them require a person.

Run a formatter on commit and a linter in CI, agree the config once in an argument that lasts an afternoon, and then never discuss it again. The specific settings matter far less than everybody thinks. What matters is that no human ever types “missing semicolon” at another human.

That alone changes the character of a review, because when the trivial comments are gone, the remaining comments are all substantive, and the author can tell.

The tells

Here’s how I’d distinguish the two versions of this, since both look like careful attention to detail.

Teaching says why. “Use find here” transfers nothing except that you should have known. “Use find here, it stops at the first match so it’s clearer about intent than filtering and taking the first element” leaves the person able to make the decision themselves next time.

Teaching is specific about praise. Not “nice one,” which is politeness. “That’s a better name than the one I’d have used” or “thanks for splitting this into three commits, it made this much easier to read” tells somebody what to do more of, which is the half of feedback everyone forgets exists.

Gatekeeping, on the other hand, displays knowledge rather than transferring it. There’s a particular kind of comment that’s mostly a demonstration that the reviewer knows a thing, with the correction attached almost as an afterthought, and everyone can feel it including the person writing it.

Gatekeeping rewrites the author’s code in the comment box, in full, so the only available response is to paste it in.

Gatekeeping asks rhetorical questions. This is the one I’m guiltiest of. “Why is this here?” from a senior person to a junior person is not received as a question even when it is one, because the power difference does the interpreting. If I mean “I think this is wrong,” the kind thing is to say that plainly. If I genuinely don’t know, I’ve started saying so out loud: “genuine question, not a criticism.” It’s four extra words and it changes what the sentence is.

And gatekeeping is unbounded. 40 comments and no approval, over three rounds, on a change that was fine after the first.

Say whether it’s blocking

The single highest-value convention I’ve adopted, and it costs one word.

Label every comment as blocking or not. I use nit: for anything optional and leave everything else plain, and I try to make sure the blocking ones are a short list.

Because from the outside a reviewer’s comments are undifferentiated. A junior cannot tell which of your 11 remarks are “this will break” and which are “I’d personally have done it another way,” so they treat all 11 as mandatory. That’s exhausting, it’s slow, and it teaches them that their own judgment has no standing, which is the exact opposite of the thing you’re trying to grow.

Small diffs are the author’s half

The reviewer gets most of the attention in posts like this, and the author controls the variable that matters most.

A change small enough to be read properly will be read properly. If you want good review, ship 200 lines, not 900. If a piece of work is large, split it: the refactor in one, the behavior change in the next, so the reviewer can see the move and the change separately rather than trying to spot a needle in a rename.

Which is the same argument as the bisect post and the commit message post, and by now I’ve noticed that everything I believe about version control reduces to “make it smaller,” and I’m no longer embarrassed about that.

Also: write the description. What this is, why, what you’re unsure about, and what you’d specifically like looked at. A pull request with “asked for feedback on the caching approach, the rest is mechanical” gets a better review than one with the ticket number in it and nothing else.

Being reviewed

And the other side, briefly, since I’m no good at it.

I get defensive. Not visibly, I hope, but I feel it, and it arrives before the thinking does. What’s helped is a small delay: read the comments, close the tab, do something else, come back. Roughly 80% of the sting is gone in 20 minutes and the remaining 20% is usually a point I need to argue properly rather than emotionally.

And it’s fine to disagree, in writing, with reasons. Review isn’t an instruction queue. If someone hasn’t understood the constraint, saying so is part of the process, and a team where nobody ever pushes back on a review isn’t harmonious, it’s quiet.

Forty seconds, five years

The 2015 comment took its author maybe 40 seconds. It’s shaped five years of my work.

I don’t think you can plan for that, and I don’t think most comments do anything at all. But the odds are much better if you’re writing to be understood rather than writing to be right, and that’s really the whole distinction I’ve been circling here.

I did apologize to the junior, incidentally. We put the line back in 🙃