They deleted the line
A question from somebody who can block your work is not received as a question, and there's no wording that gets you out of it.
“Why is this here?”
That was the entire comment. I left it on a junior developer’s pull request in the spring, next to a line I couldn’t figure out the purpose of, at the end of a long afternoon where I’d already read two other branches. I meant it literally. I wanted to know.
They deleted the line.
It was a guard on the order summary, checking that a shipping method had actually been picked before the template went and read a rate off it. There’s one narrow path where it hasn’t been, which is a customer coming back in through an abandoned cart email and landing on the summary with the cart rebuilt and nothing selected. The person who wrote that line had spent a week living in that flow. I had spent the length of one diff in it.
I found out a couple of weeks later, in the same file for an unrelated reason. When I asked, they said, “you seemed like you thought it shouldn’t be there.”
I didn’t think anything of the kind. I typed a question, and what arrived at the other end was an instruction, and nothing in the words did that. My job title did that.
Genuine question
A question from somebody who can approve or block your work is not a question. It’s a verdict with a question mark stapled to it, and the reader does the stapling, not you. There’s no phrasing that gets you out of it, because the difference in position is doing the interpreting before the sentence is even read.
So I’ve stopped asking the words to carry it. If I think a thing is wrong, I say I think it’s wrong, plainly, and then at least they know what they’re arguing with. If I genuinely don’t know, I say “genuine question, not a criticism” and then ask. It’s five extra words, it feels a little remedial typing them out, and it changes what the sentence is.
Because the person on the other end can’t tell curiosity from disapproval, and when they can’t tell, they resolve it in the direction that’s safe for them. Which is always whatever they think you want.
Bad at the thing it’s for
The standard justification for all this is catching defects, and I’ve come to think that’s the weakest claim review has.
Reviewers don’t run the code. They read a diff, out of context, with none of the system in their head, having been interrupted to do it. A narrow set of things is findable that way, obvious logic slips and missing checks and whatever falls over on empty input. The bugs that cost you a weekend come out of interactions a diff cannot show you, and half the time the thing that decides the outcome isn’t in the changed file at all. That’s the same wall I hit from the other side reading an unfamiliar codebase, where being lost is the normal condition and not a signal that something’s gone wrong.
Then there’s the size problem, which nobody enjoys admitting. A 40-line change gets thoughtful comments. A 900-line change gets an LGTM in 4 minutes, because nobody can hold 900 lines in their head and everybody knows the author has been on it all week and is sitting there waiting. The attention drops off exactly as the change gets more dangerous.
So if defect detection were really the point, this would be an expensive ritual with a poor hit rate, and I’d argue for putting the hours into tests instead. You do it anyway because it’s the main way a team moves how it thinks from the people who have it to the people who don’t. And this year it’s very nearly the only way, because nobody sits next to anybody anymore. The conversation that used to happen by leaning over now happens in a comment box, or in half an hour somebody had to put in a calendar, or it doesn’t happen.
Let the linter be the jerk
Before any of the human part, get the mechanical comments out of the process completely.
Spacing, quote style, trailing commas, import order, line length. Every one of those costs a notification, another push, and a very small piece of the author’s dignity, and not one of them needs a person. Run a formatter on commit, put a linter in CI, agree on the config once in an argument that lasts one afternoon, and then never discuss it again. The specific settings matter far less than everybody thinks. No human ever types “missing semicolon” at another human, and that matters.
That alone changes the character of the thing, because once the trivia is gone, every remaining comment is substantive, and the author can tell.
Show your work
Both versions of this look like careful attention to detail from the outside, which is most of why they’re hard to tell apart while you’re doing one of them.
Teaching says why. “Use find here” transfers nothing except that they should have known already. Google’s own reviewer guide is mostly this one idea at length, and I wish I’d read it before the spring and not after.
Use find here.
Use find here, it stops at the first match, so it says
what you actually mean better than filtering the whole
list and taking element zero. Same result, cheaper on a
long list, and the next person reading it knows you only
ever wanted the one.The second one is longer and I have never once regretted the length. The verdict isn’t what teaches, it’s the route you took to get to it, and the route is the part they can reuse on a problem I’m never going to see.
Teaching is also specific when it’s being nice. “Nice work” is politeness. “That’s a better name than the one I had in my head,” 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 counts as feedback. I’m bad at this one and I know exactly why. Praise feels like it costs nothing. So it feels like it’s worth nothing, and neither of those follows from the other.
The other version displays knowledge instead of moving it. You can feel it in a comment that’s mostly a demonstration that the reviewer knows a thing, with the correction attached almost as an afterthought. I suspect everybody can feel it, including the person typing it. You can feel it when somebody rewrites your code in the comment box, in full, so the only available response is to paste it in and learn nothing. And you can feel it when it’s unbounded, 40 comments over 3 rounds on a change that was fine after the first, which is a way of not approving something without ever having to say out loud that you’re not approving it.
nit:
The highest-value convention I’ve picked up in years, and it costs one word.
Label every comment blocking or not. I use nit: on anything optional and leave the rest plain. I try to keep the plain ones down to a short list, because a long list of blocking comments is usually me having opinions instead of me finding problems.
From the outside, a reviewer’s comments are undifferentiated. Somebody a couple of years in cannot tell which of your 11 remarks are “this will actually break” and which are “I’d have done it the other way,” so they do all 11. That’s slow, it eats an evening that wasn’t yours to spend, and it teaches them that their own judgment has no standing here, which is the precise opposite of the thing you were trying to grow.
The other half is that it makes you count. Typing nit: in front of a comment is a small act of admitting the comment is optional. I’ve deleted a fair number of comments at exactly that moment, which I think is the convention doing its real work.
The author’s half
Reviewers get all the attention in posts like this one, and the author controls the variable that decides most of the outcome.
A change small enough to be read properly will be read properly. If you want a good review, send 200 lines instead of 900. If the work is genuinely big, cut it up, the refactor in one commit and the behavior change in the next, so the reviewer can see the move and the change separately instead of hunting for a needle in a rename. Which is the argument I made about releasing 200 things at once pointed at a smaller unit, and by now I’ve noticed that everything I believe about version control comes back to “make it smaller,” and I’ve stopped being embarrassed about that.
And write the description. What this is, why it exists, what you’re unsure about, what you’d like looked at specifically. “I’d like feedback on the caching approach, the rest is mechanical” gets a better review than a ticket number and nothing else, because it tells the reviewer where to spend attention they were going to spend somewhere regardless. Same instinct as the commit message, except the reader is somebody about to give you an hour of their afternoon instead of somebody in three years trying to reconstruct what you were thinking.
40 seconds
The oldest review comment I can still quote is from 2015, on a form handler, left by somebody who has since gone off and become a teacher. “This works. Take a look at what happens if somebody submits it twice quickly, it bit us on the last project too and took me forever to spot the first time.” That took maybe 40 seconds to type. I’ve thought about double submission on every form I’ve built since, which is five years now. That’s a pretty good return on one sentence.
I don’t think you can plan for that, though. Most comments do nothing at all, mine very much included. There’s no way to tell at the time which one is going to be somebody’s next five years.
The other side of it I’m still bad at. I get defensive, not visibly I hope. But I feel it, and it turns up well before any of the thinking does. A delay helps. Read the comments, close the tab, go and do something else, come back. Most of the sting is gone by then, and what’s left is usually a point I need to argue properly instead of emotionally. And it is fine to disagree, in writing, with reasons. Review isn’t a queue of instructions. A team where nobody ever pushes back on a review isn’t harmonious, it’s quiet.
The line went back in. I wrote the reason above it this time, which took two minutes and is the only part of that whole episode still doing any work today. I can’t undo the two weeks where somebody who understood that checkout path better than I did sat around thinking they’d been caught doing something dumb, because I didn’t bother typing the second half of my own question. I’d like to say I’ve stopped writing comments like that one. I’ve actually gotten slower about hitting submit on them, which is less impressive and seems to work.