Do you have evidence that the recommendations you make actually improve things like code quality, team productivity, or other desirable endpoints?

The reason I ask is because I would prefer to work in an environment where in about 80% of your examples the team follows the “what not to do” example instead of the “here this is better” example. Maybe I’m an outlier, though; I’m perfectly willing to and even eager to have objective content instead of “opinions passed off as fact”.

I think it is exceedingly important for code reviews (and other interactions) to be respectful and polite. But very little of what you mention actually falls under respectfulness or politeness. Rather, a lot of it is tradeoffs about how to use time, whether to assume the person who is getting their code reviewed is basically competent or basically a beginner who needs a lot of training, and how to split the burden of getting team members up to speed. Despite your title, neither set of choices are “toxic”; they have different advantages and drawbacks, and either way can be entirely polite and respectful and welcoming (or can be hostile and belittling).

#### Fix to “unhelpful” behavior 1 gone wrong:

Appropriate (brief) comment:

> Subclasses have different data layout so can’t implement this properly

Belittling comment:

> This method makes assumptions about the data layout that are violated by subclasses. This is is a violation of the “Liskov Substitution Principle” (which is the foundation of object-oriented programming), <cite cite cite>.

I would much rather work on a team where the norm is to make brief to-the-point comments, where everyone is very open to dialog, than to have basic programming concepts explained to me over and over. Usually the problem is not that I haven’t heard of the Liskov Substitution Principle (and if it has, I can ask, get and answer, and now I’m up to speed); the problem is that I overlooked something.

#### Fix to “unhelpful” behavior 2 gone wrong:

  i * j / k + m

> Please use parens to clarify precedence (here and ~20 other places)

Coder thinks: _!*%!%@*^ which 20 other places?!?!_

#### Fix to “unhelpful” behavior 3 gone wrong:

+ var x=7; if ({while (x < 15 && foo(x)) x += 1; bar(x)}) {
- var x=6; if ({while (x < 14 && foo(x)) x += 1; bar(x)}) {

> It’s not clear how this code works and what problem this change fixes. Could you restructure it to a more standard style and leave a comment explaining why these are the correct magic numbers?

If someone figures out what is going here well enough to know that 7 and 15 should be 6 and 14, everyone is going to be better off if the person making the PR solves this “while they’re at it”.

This follows another well-known principle for making a pleasant community, namely “leave it better than you found it”.

I would certainly hope it would be okay to _ask_; the author of the PR should feel free to disagree that it’s necessary or reasonable. Again, open dialog is key.

#### Fix to “unhelpful” behavior 4 gone wrong:

foo(baz(5+f(x)), baz(5+f(x)), baz(5+f(x)))

> Why don’t you just factor baz(5+f(x))into a variable?

Answer:

> Unfortunately, both baz and f are side-effecting

This is way better than

> You can factor out baz(5+f(x)) into a variable, which has the benefit of reducing repetition and making it clearer that all three arguments to foo are the same.

because this comment is actually wrong due to baz and f being side-effecting (as elucidated by the question form).

And on and on it goes.

Maybe you have experience with people delivering hostility using each of the forms you’ve called out, but I would expect that mostly it’s the hostility which is a problem, not the form.

Again, if there’s data indicating otherwise, I’d be delighted (genuinely — it’s so hard to get anything solid in this area, despite it being a major part of the job!) to see whatever manner of evidence exists to the contrary.