I've long wanted to write a blog post on applying what I learned from effective communications books to code reviews.

Your comment mirrored something I wrote in another thread about the problems with the Socratic method in general[1]:

"If you have a concern, then express the concern openly before asking your question. This will make it clear to the recipient what your intent is, and they will not have to guess."

The worst comment I see in code reviews (and sadly, all too often) is:

"Why did you do it this way?"

I have no idea why I'm being asked this. The answer is "Because it solved the problem."

Even this is problematic:

"Why did you do it this way instead of X?"

Possible (unhelpful) answers:

"Because I didn't think of X." (I still don't know if you want me to change the code and why).

"Because it solved the problem."

Your examples are good ones on how to ask this question "This could have been solved via X, which has the benefits Y and Z compared to your approach. I suggest changing this to use X, unless your approach has advantages that I'm unaware of."

Probably about half the times I get something like this: Yes, my method did have advantages the reviewer is not aware of, and we then have a discussion.

[1] https://news.ycombinator.com/item?id=31889518

> Why did you do it this way instead of X?

Argh, yes, that's one line that might make me just walk away from a PR as a new/junior/casual contributor.

You, the reviewer, are an expert in the system. Likely you are the or one of the most expert people in the entire world on this exact thing. You know X exists and why to use it. As you should, because you put it there. You also should know that people who aren't experts (like me) don't know about it, simply because they didn't use it, in this PR, when they should have. Why don't they know it? Probably because you haven't used it consistently in your own code, or it's not documented. This newbie has cobbled this PR together from what sense I can make of this project. Probably 90% is guessed from code I found in there already.

What wouldn't wind me up?

"I think a better way to do this is X. It's better because Y. Or have I missed a specific reason for X?"

Note two things: 1) explanation to a noob of reason Y, which may well be valuable, not only to the noob, but also in the record of the project in general. 2) The indication that the noob might at least have had a logical approach, and they're not an idiot, just a noob.

Afterwards, if this seems like something the noob should have known from the codebase, consider that you, the maintainer, have failed to make it clear.

Of course, if I'm also supposed to be an expert, e.g. a co-maintainer, then it's different. I should know X. Which means either it's a brain fart, or I actually do have a reason. In which case I should have commented on the code, because if another contributor can't tell the intention in the PR, then they can't tell in a year when no one can remember why it went that way.

> You, the reviewer, are an expert in the system. ... You also should know that people who aren't experts (like me) don't know about it, simply because they didn't use it, in this PR, when they should have.

While this happens, I find that it's very common that even a junior developer has some insights the expert reviewer doesn't by virtue of the fact that he/she has spent more time on this specific problem than the reviewer has. As a reviewer, it's always better to assume you are not the expert.

> Of course, if I'm also supposed to be an expert, e.g. a co-maintainer, then it's different. I should know X. Which means either it's a brain fart, or I actually do have a reason. In which case I should have commented on the code, because if another contributor can't tell the intention in the PR, then they can't tell in a year when no one can remember why it went that way.

I disagree, and your example is a good case of not considering all the possibilities.

Yes, in certain cases, X may be the obviously better approach, and a comment would be a good idea. In many/most cases, there are N approaches, and you happened to pick one. If your approach has advantages over X, I don't see a need to justify it as a comment in the code because you then should put comments to justify it over the (N - 2) other solutions.

At the end of the day, the burden is on the reviewer to specify why he prefers X. Only if he presents a case ("X is better because of reason Y") should the developer feel obliged to justify the decision.

> In many/most cases, there are N approaches, and you happened to pick one.

I'm not sure that's always true. A lot of the time, while there might be, in a vacuum, many ways, in the context of a specific project (or part of), there's usually one "most right" way to do it, considering local style, idioms, norms and conventions. Admittedly, this might be more true in some languages and applications.

Sometimes there are multiple ways. Usually this means that one or more disparate legacy ways are extant, but there's a preferred way to move towards.

Having PRs storm off in "exciting" new directions, technically correct or not, is usually a bad idea once that merges and is now everyone else's problem.

If all alternatives are equally valid logically, it's likely they have practical implications that differ. Perhaps one might consider a std::set better than a std:: vector in the abstract for your task, but maybe you know something important about the expected number of items or memory access patterns. This is when you should comment if that's not obvious to someone who isn't literally just done writing the change.

> At the end of the day, the burden is on the reviewer to specify why he prefers X.

If a reviewer is going to pull me up on truly equivalent things that have no impact on correctness or maintainability, then I'm probably not going to voluntarily be a co-maintainer with them. If I've consented to be a co-maintainer, then I have either mutual respect for the others, or I'm being paid enough to make it worth it. And indeed, I have refused maintainership offers in projects with ":eyeroll: why didn't you just magically intuit X" cultures.

> "Why did you do it this way?"

While the question is literally (and without any other thought) what I have sometimes in mind, usually I ask something like:

I feel I might be missing some insight or context, could you walk me through your rationale?

Because if I don't get some code change I may very well be missing something. And in any case by talking we're only going to improve our mutual understanding, both of the codebase and of each other's thought process.

Seeing an open source project's reviewer begin with "Why did you do it this way instead of X?" is the the most shocking shutdown I have seen this year.

Buddy, you let that issue languish for a year. How about let my junior dev friend have a go without being a nitwit reviewer?

> Seeing an open source project's reviewer begin with "Why did you do it this way instead of X?" is the the most shocking shutdown I have seen this year.

Thing is, in most cases it's not meant to be nasty at all. Their intent usually is to point out they think there's a better way, and are either trying to say you should do it that way, or are trying to invite you to give your reasons. The problem is they're phrasing it in a manner where there are multiple interpretations to their intent.

Given that commenters responded negatively to it - beyond what I had originally meant when I highlighted it - makes it a good example of how not to communicate.

Just wanted to chime in and say that I still remember the first code reviews I received and while I didn't take any of the comments personally, they certainly didn't feel great to receive. (I should add that I don't begrudge the reviewers, they were nice people).

These example notes are wonderful. They feel like an editor's notes, not a graded exam. Something you'd get from a colleague who is collaborating with you and not some infallible black-box oracle.

Thanks!

+1 the reasoning is the entire point and all of these examples are good.

Just telling people to make changes without telling why doesn't teach them anything - especially when sometimes what's being requested isn't actually better (or is at least subjective).

my rule is to weigh whether or not its worth making a comment very carefully. nothing without a clear justification. never say 'I would have done it this other way' - unless its a good friend who you often discuss style issues with. comment on things that you feel would make a substantial difference on schedule, maintainability, or function - and provide a clear reasoning with which to argue against.

wasting your peers time trying to enforce your style isn't just non-productive, it sours everyone on the whole exercise.

These suggestions hit the nail on the head. Additionally, the reviewer also becomes a better dev because they’re forced to really think about and articulate why they think the code smells.

Even if my coworker’s code doesn’t feel right to me, I’ll sometimes forgo commenting if I can’t articulate why. My goal is to avoid bugs and deliver features, not to force my code style and opinions on others.

Agree with adding additional reasoning, regardless of the actual tone.

I’ve received some feedback along the lines of “LOL Wut?” on a comment in code, which I really didn’t know how to take or respond to (suffice to say its meaning was vague, and generally unhelpful).

Sometimes you catch people in low energy or bad moods. It doesn't excuse the behaviour but try not to take it personal.

My trick is to write the wtf in pending review then go and look later in the day if it's still valid. Often I just needed to eat something and I'll be over it.

I would talk it out with the person. Such comments aren't helpful at all, unless the mistake made on that line is obvious.

I agree with the tone of your suggestions, but your suggestions still take quite some time to read. They can easily be shortened without losing any information. For example, no need to to apologize for being the process person (being overly apologetic decreases the value of apologies anyway).

Also for me it feels like you're seeing code reviews as a senior/junior thing. It's more than that, no?

.. and sometimes after going through that exercise you might find you don't have a good reason for what you're asking for. :)

Where possible it can also be useful to point out if there are existing guidelines/checklists the submitter should be referencing to understand these preferences of the project. The more the contributors can code review themselves prior to submitting a PR the better.

And if such a resource doesn't exist or that topic is not in the current guidelines/checklists - offering to take up the task of creating or adding an topic to the list of things to check before submitting a PR can be helpful.

Eliminating nitpicky PR comments by having pretty verbose coding guidelines as well as pretty strict settings for automatic code linters/syntax checkers/static type validators means the PR feedback cycle is a lot more focused.

And for architectural discussion - draft PRs can be useful early on for feedback on design choices for more challenging features can be helpful.

This!

Walking on eggshells because every comment chain devolves into religious battles about style? Find a linter or style checker that can automate comments/fixes. Talk about those checks in your team meeting, get consensus (who cares what it is, just consensus), and make the tools enforce it. Or suggest your manager do this.

If discussing actual code problems in code reviews rankles people, just escalate to your manager.

If you need a special code review human language lexicon because people can faint or throw punches, it's a management problem. Because that's the kind of thing that makes other people, not the troublemakers, leave the company.

I ignore style check nitpick comments.

Fix your linter and autoformatter.

You don't pay me to lint code.

These work and sound great, but stating them without a "please" can come off as a little brusque. Please add a "please" before the orders, and I'll accept the advice.

I agree with this. Don't give demands, give reasoning and examples.

Great post! One quibble with your last point — your guide is probably most helpful to the devs that are too direct and blunt, and expect others to be that way to them.

Haha! That's an excellent way to state the issue.

Yep: the operative word being “because”, absent in all the OP’s sentences.

The original post seemed to be concerned about tone, not content.

That’s the point: the replies are saying that the content is more of an issue than the tone, and so OP should focus on the hints for the former.

How do we know the OP had any issues coming up with valuable and insightful comments? Tone can have an impact - being particularly rude is not likely to result in good outcomes (but maybe it could in some situations). Why aren't we exploring what was originally asked?

Because all of the versions have the same problem regardless of tone. If the assumptions are in error and the OP had been leaving off the justifications suggested, they can reply and say so! (But, as it turns out, didn’t.)