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

My rules for reviews (which influence the language used) are:

* Criticise the code, not the author ("There's a bug here" vs. "You inserted a bug here");

* Reach common agreement that people shouldn't cling to code written ("sunk cost fallacy"). Code is liability, and it's okay to throw things away and restart from scratch, the value is in the learning;

* Don't use questions that invite discussion if you're not inviting a discussion, instead expose the reason you believe something should be different ("Should this be like X?" vs. "I think this should be like X for reasons A, B, C."). This avoids wasting time and reviews that turn into long discussions.

* Don't give orders ("Do this", "Do that") to your colleagues, since they are not your employees, and it doesn't help promote learning & growth by not exposing the reasoning behind the demand;

* Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools;

* Agree previously on what constitutes a non-passable review, if there's such a process.

It's about power dynamics.

You ask someone above you. You tell someone below you. You discuss with someone on the same level as you.

If you're smart, you'll quickly spot the assholes who think they're above others - you probably shouldn't comment on their code at all, other than 'great, looks good'.

If you think someone is below you, you're an asshole.

If you want to discuss, you need to include the word 'because'. 'Hey, can we do X, because I/we will need it for Y?' is the way I usually go about it. By the way, discussions are best had before anyone writes code, not after. So for me, I don't do code review - if someone's bad enough that they need their code reviewed on a regular basis, I will be leaving soon anyway.

> * Should we extract this to a separate function?

Reads very passive aggressive. No.

> * Could you extract this to a separate function?

Yes, I could.

> * I would extract this to a separate function.

Good for you.

> * This could be extracted to a separate function.

This one's OK as a style suggestion that you won't block the merge over.

> * This should be extracted to a separate function.

This is kinda, sorta OK, if you're going to block the merge if this isn't fixed.

> * Extract this to a separate function.

This is better because the tone matches the intent: it's a command. This gets addressed, or no merge. "This should..." is indirect—you're not stating what you actually mean. This phrasing is better.

4 is the best of these for optional suggestions, 6 is best if you consider the fix a requirement for a merge. In general, say what you actually mean—dancing around it leaves room for miscommunication and can give all kinds of bad impressions. Of course, that can also mean weakening language: don't command a change that you don't consider a big enough deal to block the merge.

[EDIT] And, as others have noted in the thread, of course expanding with justifications is always a good idea. "Do this" without a "why" should be avoided. But lead with what you want done, not the why—that follows.

This one is nice.

I always try to make suggestions as questions because it's less threatening, gives more agency to the author, and oftentimes ends up being a discussion on the merits. If the suggestion really is a blocker, I'll be a little more direct, but always always friendly.