Ask HN: What tone to use in code review suggestions?

There are different categories of things you might see:

1. This is wrong. I can tell from reading your code that it doesn't do what the description of this PR or the name of a test or some other indicator shows its supposed to do. This should be communicated in a tone of "you must not merge this".

2. This violates our agreed-upon style or best practices. The strictness of enforcement is part of that agreement and company culture. At my current company, this would be communicated along the lines of "this is not how we prefer to do this, so unless you have a good reason why our standard method is wrong, change it"

3. This is confusing to me, or I have a suggestion for improvement. This should be communicated as a suggestion or general feedback. If you're getting miffed about people not taking the suggestions, maybe it's actually in a category above, or maybe you need to adjust your own assumptions about how much stylistic consistency to insist on, since it sounds like you don't have a consensus.

4. You are new, either to software engineering or at least to this company, and your style is inconsistent with how things are done in this code. This is the same as #3 but should be communicated more strongly and depending on your company normals may be effectively a requirement.

The wording above could be improved for sure. But the intention is to shift focus away from telling somehow how to solve a problem to forming consensus on whether there's a problem at all. And that starts with the reviewer expressing a concern rather than immediately providing a solution. "Is a change needed?" should come before "this is a needed change".

A better example might be "I'm seeing this same code in various places--do we benefit from keeping them distinct?" or "you took an inheritance approach here, but I'm concerned about the long-term impacts as the codebase changes. How do you think this code compares with a composition approach?".

If in doing so you would think that I'm being sly, that's unfortunate, but there's not much I can do in how you interpret stuff. I can't control for others assuming bad faith. And if there's bad faith, there are probably communication and/or trust problems amongst the team members that are broader than code reviews, and is something that should be tackled directly.

+1 for use of Conventional Comments. It's a simple lift for a noticeable improvement in clarity, and guidance on what to do next.

Code review is a form of communication, and thus depends very much on culture.

This can both be company culture and origin culture of the developer you're communicating with.

For example, sometimes I write comments like "I don't particularly like this because" plus an explanation, plus "but I don't see any significant better way, so I'm fine with it for now". Some of my colleagues are fine with that, maybe they add a code comment like "TODO: find a way to fix problem X", or they just acknowledge and don't change anything. And then there are developers I work with that come from a different country, and they'll spend another half day or even two days coming up with a better solution, or don't dare to click on "resolve thread".

I think their culture is really geared towards avoiding verbal criticism, and if I even bother to write something negative (and maybe I'm also senior to them, in the org chart), it must be really meaningful and must be addressed.

So for some of these "foreign" developers I only leave comments where I expect something to change; for others that whose culture I better understand, I sometimes leave comments that are more conversational.

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