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

The tone also deeply depends on the culture of the receiver.

Depending on whether their cultural background is North American, Scandinavian, German, French, Italian or Chinese (to pick from the very situations I have faced), and especially if they are more junior, you cannot expect the same outcome from the same feedback, without additional context/care in the communication.

You've done a good job here, but I might suggest stepping back even a bit further. If you want reviews to work, they must be in the spirit of "we all improve and learn together" rather than "bad dog! bad!" The tone you set in why you are doing them, who is involved, how they run makes all the difference in the world. What you want is to create a culture where it is safe to point out something that could have ramifications, and to promote a culture where there can be dialog about what's best for the health of the team and products you support. More often than not though, I've seen these meetings turn into "beatings shall continue until morale improves" sessions.

Just my $0.02.

Lots of good suggestions.

I don't think there are simple answers, and it's something you'll have to figure out separately for every reviewee, and sometimes every review.

You have to consider power dynamics, preexisting relationships, your own time, the importance of the change, and cultural differences.

If the power dynamics are too far apart, the right answer is often "don't". A high-level engineer or architect reviewing a relatively junior coder's patch should not be commenting on anything involving taste or stylistic suggestions. They should be reviewing for functionality, security, compatibility, etc. And only bring up maintainability or performance when it really makes a difference.

Some suboptimal code will get landed as a result, but it's better than creating a culture where the senior's personal preferences are always going to win out. When there's a power imbalance, nuance gets lost—the junior either has to bend to the senior's preferences even when they have a good reason to do things differently, or they have to go to a lot of effort to justify their position. Which is fine in some cases, but it's a waste of time and effort for things that don't matter as much. (This isn't about avoiding hurting people's feelings, by the way, though it does serve that purpose as well.)

I also agree that pointing out positive qualities of a patch can make up for quite a few critical comments. Explicitly constructive criticism (eg sketching out an alternative, or doing some work to provide data or justification for a comment) can also "buy" a smaller amount of more negative-sounding comments.

And of course, if you're swapping patches with someone regularly, there's no need to overthink it: state the problems you see, the things you're not sure about that concern you, the suggestions you have, etc., using as clear and concise language as possible.

In all cases, do not make the reader guess your actual opinion or intention. If you're couching a criticism in gentler language to avoid making them feel bad, that's fine, but don't play games or leave out important pieces. There's always a way to coach or criticize honestly, and you'll do yourself a lot of good by finding it.

My usual approach is to point out the problem I'm trying to solve rather than suggest a specific solution. I also like to phrase as a question about practicality. Sometimes the answer to that question is "No", and I want to make sure that even someone very junior is comfortable giving me that answer.

- This code looks pretty similar to this other code; is there a way it can be deduplicated?

- I don't understand this code; I think it's because there are too many conceptual steps. Is there some way to structure it for better readability?

- Is there a way to unit test this specific behavior?

Sometimes this style doesn't work (e.g. correcting a comment that has become incorrect due to the change), and in those cases I usually just say "Please do this." I also try to point out things I like when they are present.

I mostly only ever use the form: "This should be ... " with a possible explanation why. We're all mature enough to not take it personally, so I'm not going to beat around the bush by writing a lot of proza. The imperative of the last example ("Do this") leaves no room for argument (because I might be wrong).

Either that or I ask for clarification of a piece of code: "Why did you do this instead of this?".

Like already stated by someone else; your approach really should depend on the team you're in. I'd take a less direct approach with junior devs who tend to be a bit more insecure (and stubborn).