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

My suggestion would be typical: concise, professional, and when straight-forward, specific. The key is to remember that the nature of a code review is first to improve product, and perhaps secondarily to improve process (ops or procedure) and perhaps thirdly, people improvement in the form of collective learning.

Thus, if we're talking about refactoring a line-of-code then just writing the refactored line-of-code is probably the most clear and concise. If it's more involved than that, then it may be worth taking offline. In the example you've provided here, I'd suggest the best language is likely: Refactoring this block as a separate function provides the following benefits: 1. blah, 2. blah blah, and 3. blah blah blah and no side effects. The only additional clarity that's helpful is to say whether it's optional, required, or some other directive.

> "Why did you do it this way?"

While the question is literally (and without any other thought) what I have sometimes in mind, usually I ask something like:

I feel I might be missing some insight or context, could you walk me through your rationale?

Because if I don't get some code change I may very well be missing something. And in any case by talking we're only going to improve our mutual understanding, both of the codebase and of each other's thought process.

"Don't repeat yourself (DRY)."

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

I'd love some suggestions for how to get people to avoid this type of bike-shedding. While I'd love for us to use some tools to automate this, we have a huge codebase with existing code that doesn't follow the rules, and it's not in my ability to implement said tools. (Big company, lots of overhead, will affect a lot of people, don't have a lot of time, not my department, etc.)

One of the things I hate is having a different comment for every single place where I misplaced a star or ampersand. My natural tendency is to do it one way, but our style guidelines prefer it the other way, so it's something I miss frequently. Having 30 comments in a review that are all "move the star to the other side" over and over again just makes me hate the reviewer. (Particularly if they don't provide any useful feedback.) A single comment on the first one that says something like, "This doesn't match the style guidelines, and I see several others that don't match. Please fix them all," would be highly preferable. And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!

We do something very similar:

* NICE: Optional change. PR is approved.

* SHOULD: Highly suggested change. PR is neither approved nor rejected.

* MUST: Must be fixed. PR is rejected.

This also helps the PR author to know which of my comments have to be addressed to resolve my rejection.