They deleted the line
A question from somebody who can block your work doesn't arrive as a question, however you word it.
“Why is this here?”
That was the whole comment. I left it this summer on a pull request from a developer on the outside team that builds a lot of our front end now, next to a line I couldn’t figure out the purpose of. It was the end of a long afternoon and 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 that made sure a shipping method had actually been picked before the template went and read a rate off it. There’s one narrow path where nothing’s been picked yet. A customer comes back in through an abandoned cart email and lands 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, when I was back in that file for something unrelated. I asked about it and 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 an instruction arrived at the other end. Nothing in the words did that. My position did.
I review that team’s work on this client’s store, which makes me the last stop between their code and the client. Every sentence I type goes out with that attached.
Genuine question
A question from somebody who can approve or block your work isn’t a question. It’s a verdict with a question mark stapled to it. And the reader does the stapling, not you. No phrasing gets you out of that, because the difference in position is doing the interpreting before anybody’s even read the sentence.
It’s worse across two companies. They’d never met me or heard me say anything out loud, so there was no tone to read the comment in except the one the approve button gave it. And their morning starts at about the time my day ends.
Asking me what I meant would have cost a full day’s round trip on a ticket somebody was already waiting on. Deleting the line cost them a minute and got the pull request approved. I’d have deleted it too.
So I’ve stopped asking the words to carry it. If I think something’s wrong, I say plainly that I think it’s wrong. 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 and it feels a little remedial typing them out. But it changes what the sentence is.
That matters because the person on the other end can’t tell curiosity from disapproval. 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. 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 only as much of the system in their head as they happened to bring. And they got 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 can’t show you. Half the time whatever decides the outcome isn’t even in the file that changed. It’s the same wall I hit from the other side reading an unfamiliar codebase, where being lost is the normal condition and not a sign 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. You do it anyway because it’s the main way a team’s knowledge gets to the people who don’t have it yet. With an outside team it’s close to the only way.
They’ve never talked to the client or sat in on the call where the checkout got its odd rules. Nobody walked them through the abandoned cart emails. Nearly everything they know about this store that isn’t in the code reaches them through my review comments.
This year that’s true inside our own team too, 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 at all.
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. Not one of them needs a person. Across two companies and a time difference, each one also costs a day.
Run a formatter on commit and the linter before anything goes up for review. Agree on the config once, in an argument that lasts one afternoon. Then hand the config to the outside team on their first day and never discuss it again.
The specific settings matter a lot less than everybody thinks. No human ever types “missing semicolon” at another human, and that part does matter. That alone changes the character of the whole thing. Once the trivia’s gone, every comment that’s left is substantive and the author can tell.
Show your work
There are two versions of this. From the outside they both look like careful attention to detail, which is most of why they’re hard to tell apart while you’re doing one of them.
Teaching says why. “Use find here” doesn’t transfer anything except that they should have known already. Google’s own reviewer guide is mostly this one idea at length. I wish I’d read it before the summer instead of 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’ve never once regretted the length. The verdict doesn’t do the teaching. The route you took to get to it does, and that’s 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. That’s 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 tacked on almost as an afterthought. I think everybody can feel it, including the person typing it.
You can feel it when somebody rewrites all of your code in the comment box, so the only thing left to do is paste it in and learn nothing. And you can feel it when there’s no end to it, like 40 comments over 3 rounds on a change that was fine after the first. That’s a way of not approving something without ever having to say out loud that you’re not approving it.
nit:
This is the most useful 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 all look alike. Somebody at another company, reading you from half a day away, can’t 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 and it adds a day to every round. It also teaches them that their own judgment has no standing here, which is the exact opposite of what you were trying to grow.
The label works on the reviewer too, because it makes you count. Typing nit: in front of a comment is a small admission that it’s 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. But 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, with the refactor in one commit and the behavior change in the next. That way the reviewer sees the move and the change separately instead of hunting for a needle in a rename.
It’s the same argument I made about releasing 200 things at once, pointed at a smaller unit. By now I’ve noticed that everything I believe about version control comes back to “make it smaller.” 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. It tells the reviewer where to spend attention they were going to spend somewhere anyway. 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 piece of review I can still quote never went near a pull request. It was 2015 and there was no version control to speak of. Somebody who has since gone off and become a teacher was standing behind my chair while I tested a fix to a practice’s contact form.
“This works. Take a look at what happens if somebody submits it twice quickly, it bit us on a dentist’s site last year and took me forever to spot the first time.” That took maybe 40 seconds to say. I’ve thought about double submission on every form I’ve built in the five years since. 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.
I’m still bad at being on the other side of it. I get defensive (not visibly, I hope). But I feel it, and it shows up well before any of the thinking does.
A delay helps. Read the comments, close the tab, go and do something else, come back. By then most of the sting is gone and I’m usually left with a point I need to argue properly instead of emotionally.
And it’s 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 when somebody who understood that checkout path better than I did thought 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 but seems to work.