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

I would say though that a lot depends on team dynamic & hierarchy. For example, I have no direct reports, so I'm never going to use the imperative as in 'extract this to a separate function' - taken literally, I don't have the authority to tell anyone to do that.

Most often I think I favour 'I would', and give a reason. I.e. I found this a bit surprising, because I would have done it differently.

Along the same lines I also ask (honest) 'is there a particular reason' or 'why not' type questions. Something else seems most obvious to me, but maybe they thought of (even tried) and dismissed that.

"Maybe it would be better…"

"IMO we should do …, what do you think?"

You really want to be on this person side and help them achieve. That's your job after all

If you write that you clearly think it's problematic but hide it behind an unnatural question. Then either I do what you hint at or you will push / discuss until I agree. I am not the determiner. It sounds like you are schooling me with an awkward back and forth.

Honestly, to me this is beating around the bush and doesn't actually make me confident we can have a real discussion (in the context of that code review).

"Do you perhaps think that doing this is problematic?" sounds like either you want to passively insult me, or you don't consider me a peer with whom to have an actual discussion on equal footing, or you don't have the guts to share your opinion without contortions.

There is no offence to be taken, because the deeply held respect for each other as team mates is already there and taken as a given. You trust each other to know that any comments are addressed directly to the code, rather than to the person behind them. All input is valued because everyone knows the goal is just to make the code better and help each other out, rather than criticise an individual. Discussion can be frank, open, unbiased and non-confrontational.

However, because that takes a long time to develop and is somewhat of a rarity, I've had quite good luck with the phrasing:

"Have you considered X here?"

It has a number of advantages:

* It doesn't imply that an author should have done it a different way

* It doesn't imply that an author has missed something you think they should have covered

* It's a good starter for a conversation, rather than a conflict

* If the answer is "no", it provides a good prompt for an author to come up with a suggestion first

* If the answer is "yes", they can explain why they ended up with the solution they did, which is visible to everyone who wants to look at the PR. That can also be a good prompt to add a comment

"These lines have been repeated a number of places and probably ought to be pulled into a function called something like xyz in module zyq"

Generally your tone doesn't matter as long as it's not mean. "This is the 10th time I've told you declare constants FOR EVERY MAGIC NUMBER," does not belong in a code review; it belongs in feedback.

However there is something a bit dangling here which is linting and static analysis. If code has been repeated and genuinely ought to be pulled into a function, static analysis ought to be able to find this.

If you have already dealt with the broad class of possible change requests that can be caught by static analysis, the nature of the reviews should get a lot more straight forward. It will stay at a higher level and naturally tend toward verbiage like "I wonder if we ..." or, "I was thinking if ..."

In cases where static analysis would not find it for e.g. in the middle of a function doing something, we have several lines of code collecting data for some analytics event which is never repeated, but still confusing; I would say something like "For readability, could we pull the analytics bit into a separate function?"

Generally; if you can use "we" instead of "you" or "I"; I would prefer "we" in any team setting.