No, for several different reasons.

It is often hard to tell the difference between great code and bad code. I've written code that was fast and very flexible, at the expense of being complex - the flexibility turned out not used at all (after 8 years in production I'm confident it never will be), so on hindsight this wasn't great code - but if the flexibility envisioned had been used the complexity would be required and the solution would be great. I've seen code that cleverly handled multi-threaded issues such that can't be understood or modified by anyone without breaking - but it great code because it abstracts the threads so nobody else has to understand them.

Second, when you first start on a project you don't understand it all. We want to grow your understanding and bring you in as trusted contributor. Setting the right tone in those first days makes all the difference in the question of will you contribute more code or not.

Last, most open source projects are essentially unmaintained. Almost any contribution is better than that, even if the code is bad, if it works at all...

> It is often hard to tell the difference between great code and bad code. I've written code that was fast and very flexible, at the expense of being complex - the flexibility turned out not used at all (after 8 years in production I'm confident it never will be), so on hindsight this wasn't great code - but if the flexibility envisioned had been used the complexity would be required and the solution would be great. I've seen code that cleverly handled multi-threaded issues such that can't be understood or modified by anyone without breaking - but it great code because it abstracts the threads so nobody else has to understand them.

At my previous company, the lead dev did this (the complex code part with the lack of usage too), unfortunately his view was that it might be used sometime.

I think, after 10 years, we were handling about 100 events per second peak load and what I pointed this out he countered with: What if we had to handle 100000 or 1 Million?

I gave up after that.

For context we were a B2B company with a relatively long sales cycle, so ramp ups were generally very strongly correlated with on-boarding of new customers and we normally had notice of what was coming. In other words, the naive solution would've worked fine ( the 100 events (not web) were across ~ 5 servers (servers did other stuff too)) and we would've been able to scale up while we did improvements to the naive solution.

This advice is similar to the Hacker News Guidelines [1]

Such as:

> Be kind. Don't be snarky. Comments should get more thoughtful and substantive, not less, as a topic gets more divisive.

or

> Please don't post shallow dismissals, especially of other people's work. A good critical comment teaches us something.

and

> Please respond to the strongest plausible interpretation of what someone says, not a weaker one that's easier to criticize. Assume good faith.

Being kind doesn't mean being a push over. Rather, it makes it easier to achieve the goals you want.

None of these suggestions advise you to accept less-good code. Rather, it's all about making it easier to get better code.

I could have also just said "Can't you see that you are obviously wrong?" and left it at that. Would that have been better?

1. https://news.ycombinator.com/newsguidelines.html

In my experience, this is not the case when both the reviewer and the reviewee are working together and want to find a good solution.

Rephrasing criticism is a pretty common technique not exclusive to code reviews and imho helps avoid a defensive stance which makes it easier to be productive.

The idea is to express the intent without the emotion. I don't see any watering down there; in fact, it's likely the criticism will be clearer, more direct and more palatable.

Your comment is factual untrue. Most advice is actually removing reviewers emotional reactions in exchange for more factual statements. The only watering down is that it gives benefit of the doubt to reviewed code where difference of opinion may be the issue.

The first comment actually means a strong opinion. Impossible usually really means impossible from technical standpoint and is not used for things that are heavily inconsiderate or inconveniencing future development. The suggested option is Machiavellian and patronizing at the same time, and pushing burden of proof on the author of the code instead of reviewer. Puts them on defensive. Both are evil, but first is clearly an opinion, second is also a personal attack.

Proper review really must have "X because Y" form with perhaps exception for trivial syntax fixes. Like form 5.

Part two also forces additional burden of proof on the author. Generally for performance changes documentation requirement is the only thing. Preferably runtime numbers or profiles rather than abstract notions. We've seen people going for algorithmic gold plating where it actually list performance to constant factors.

Doing 6 in excess is viewed as patronizing. There is no good way around it.

And finally, presuming you know the teammates making the review as bland as possible without a bit of color can be the worst thing as reviews will be felt as a chore. They are one still but humor can help. Some people like their reviews to be ultimately super boring instead, and you have to be careful.

I don't think OP wants a project to accept anything "less-good". It's just a post asking to be respectful to the contributor. Attack the idea(in a civilized manner) - not the person.

Seems to be advocating more for thoughtful criticism (and meaningful discussion) than anything. Is it going to make things better? To what degree, I don't know, but I'd imagine it helps with people creating pull requests and then disappearing from the project (which I've seen multiple complaints of this behavior over the years). It'd make them feel more of a stake in it I'd imagine if they're not receiving blowback from the maintainer for daring to create a pull request (based on the "bads" in the examples).

I've personally never seen anything as bad as the examples given (I've read stuff from Linus that are there, but seems like he's trying to change the atmosphere there now), but if the communication is that prickly, I can't imagine it is going to receive a ton of outside help in the form of pull requests (that could be intentional though).

I didn't read any of these as watering down. Which ones do you think dilute the message?

Exactly. I seems like most of these are jumping off points for starting a conversation.

I may be wrong but I read the article as a senior reviewing a junior's code, in which case the big win is not just getting better code in now, but getting a better developer. Starting the conversation instead of jumping all over them is important.

Even when reviewing a peer, while more terse we tend to phrase things in questions, "Shouldn't this be in a lib instead?" Frequently it's oversight on their part, but sometimes they have their reasons and I learn something in return.

"How do we do X with this change?" is less clear if what you really mean is "This change makes X impossible". "Thank you for your contribution" is misleading when talking about a contribution that simply doesn't meet minimal quality standards. "Please" suggests that you consider something optional, which is misleading if you are actually expressing a requirement. Snide or hyperbolic remarks convey a message; perhaps not a welcome one, but one whose meaning is presumably intentional.

This assumes that criticism produces good code. If only it was that easy.

I believe the idea is clear communication and constructive criticism.

(or possibly to avoid being sheldon in the big bang theory)