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

When writing suggestions in code reviews I have used all of these forms:

* Should we extract this to a separate function?

* Could you extract this to a separate function?

* I would extract this to a separate function.

* This could be extracted to a separate function.

* This should be extracted to a separate function.

* Extract this to a separate function.

As you can see, these have very different tones and I would like to be more consistent and as constructive as possible. Is there some general best practice for this? Are you or your team using a set of rules or guidelines?

I feel like this is often overlooked.

One morning, I did a code review before having any coffee. I basically used the latter two throughout the whole thing. I came into the office to find my coworker literally crying. Later in the day, someone else said it was the best code review they’d ever read… and asked me to come to their team…

There was so much drama from that code review. That code got merged as-is to appease the author without a single one of my statements addressed (including security considerations), and no one else would review it. My manager was in a tight spot, felt sorry for him.

I was literally just my pre-coffee blunt self. I usually use the first few versions in your list, which is probably why it hit my coworker so hard. We chatted and made up, but our relationship was weird after that.

So, I’d say, just be consistent. Changing tones unexpectedly might affect your coworkers and politics in interesting ways that you might not expect.

Personally, I prefer "What do you think about extracting this into a separate function?"

Unlike in any of your examples, the subtext is: "I anticipate you had reasons for doing it the way you did and I am open to listening to them." It engages the author on the same footing, leaves the door open for discussion but still communicates your intent in a straightforward manner.

Personally it all depends on the rest of the comments in the review, if you use one style all the time then you lose the ability to indicate which changes are the ones you feel are more important than others.

I'd use the more question style comments for the minor changes, and save the stronger comment style for those changes that are really important.

So I'd word a comment about changing a variable name to something slightly better as a question. e.g. Would accounts be a better name for this array?

While a comment addressing a security concern would be a direct statement. e.g. Use a parameterised query here, as it'll be harder for someone to exploit.

One thing to note is that when making the strong statements I like to back them up with a reason, that way it helps me to think through the comment itself, helps a more junior developer understand why they should change it and helps more senior developers to not take the comment personally.

The other side to this though is that as senior developers we need to set an example for our more junior colleagues on how to take review comments. I always try to take them in a good mood, make the changes when I agree with them and have a constructive conversation about those that I don't agree with.

[Suggestion] Use conventional comments[1] to flag how important the comment is.

Most of my comments end up being "suggestions", but then when I put a [blocking] on it, it clearly communicates that I think this should be fixed before merging in.

1: https://conventionalcomments.org/