Tips for reviewing code you don’t like
developers.redhat.com
developers.redhat.com
I've gotten PR reviews with people literally writing "I don't like this" even after I wrote a very detailed technical argument about my reasoning. My only approach to that is to remind them that I really don't care about their personal preferences, because no discussion will come out of them addressing none of my technical arguments and instead relying on what their personal preference is like.
I've gone into code reviews where my reaction began with, "ugh, this is terrible", but evolved into, "well, I probably would have done it differently, but this approach handles an edge case I hadn't considered".
I almost do the opposite of this advice -- refrain from criticizing unless I can show precisely how I would prefer to do it and why I think my way is better.
Code reviews are not coding exercises. You ought to be able to explain using human language primarily. The only only instances where I've actually written code in my comments was because I was reviewing Scala code written by a very old person who had worked 20+ years in Java and it was very very difficult to get some points across - and only after several tries where he didn't get what I meant.
Now admittedly I mostly work on projects where I'm the sole developer so structured code reviews aren't part of my daily routine, but during the period where I managed interns we'd do code reviews before every merge which mostly involved sitting down and going through the functions. I'd point out deficiencies and work to teach the student developers new tools/libraries/resources they weren't aware of. At no point did we exchange shit like "This change sucks", "Can’t you see that this is obviously wrong?", or "This is stupid code written by a stupid person."... all examples from an article which is also toting the "Avoid hyperbole" mantra. Even a single one of those snide remarks would be grounds for discipline or potentially termination if it were consistent.
My tip? Separate your ego from your code. If you can't discuss criticism objectively and professionally you shouldn't be involved in a professional career. Hostile workplaces are shit to be part of and I will never stand for that among my peers.
It wasn't just me, several of my coworkers felt the same about just getting hammered on by them.
This has kind of permanently changed my approach to code review. I always call out the things I like in a batch of code. I acknowledge that there are some code styles preferences that are arbitrary, but that I have a preference in and it's good to have a consistent codebase. If the person is working for me, or if a coworker took a nasty task that nobody wanted, I thank them for the code with the LGTM.
Same things go for automatic code-formatters. If every file has to be formatted according to the canonical formatter, then you remove tons of chances for nitpicking. Your code doesn't match what the formatter outputs? You can't send your code out for review.
That way it's not personal; you're not saying "my way is best", you're saying, "this is OUR way".
There is so much code-review you can do before it hits a PR. But then the PR invites nitpicking, over-thinking, and mistaken assumptions. The PR was good when it was submitted but the consensus has all manner of tweaks that are, most of the time, stylistic. Only rarely do you get a serious issue and it gets drowned out in the noise.
This isn’t separating ego from code, it’s accepting that maybe you can do peer review on your code before the fact and any requirement to approve/comment after that must offer something more insightful.
I heard this from a therapist: "everyone is just trying to feel safe and loved". It really helps me understand and empathise with people in conflict situations ("how is this action helping this person to feel safe and loved?"). It might help with understanding the code reviews too ;)
1:1 code review in person, sort of like pairing at that point. 1:1 remote via PR, 1:N in person in front of a screen or projector, 1:N remote where you need multiple approvals and different sections may interest different people, and N:M where a small team worked on a feature and request another teams review.
I think of review broadly as the remote/pr kind. But all of these are valuable in different situations.
Tact takes effort and discipline, just like programming. To draw an analogy, maybe it's like when a dev foresees a bug but can't figure a good way to address it (or isn't motivated to address it, or doesn't have the confidence, isn't fully convinced it's really an issue, &c) decides to turn a blind eye and figures it'll get resolved further down the line if it becomes a serious problem.
I actually don't like this. Maybe it's just me, but I always read these as being condescending, written by someone who thinks that I won't take a suggestion without unnecessary pleasantries and/or begging, or they're frustrated and trying to mask it.
> “Could you please align these variable definitions so they’re easier to read?”
> “Could you align these variable definitions so they’re easier to read?”
I like the latter.
Even if it's some personal style thing, I don't perceive "Could you...?" as rude or anything.
If anything an exaggerated please could seem more insulting, as in "Could you please just do this simple thing ffs!?"
Anecdotally, in my team which spans individuals from each continent except Africa and the poles, US english speakers are the least likely to use 'please', whereas our APAC (asia-pacific) contributors use it liberally.
If it's really something subjective I would say "This would be nicer if..." or "I think that could be changed to..." Instead of formulating some almost passive-aggressive request with "please".
Of course, trying to accommodate everyone's unique perspective and behaviors would never scale, so instead we fabricate social norms that represent a sort of common denominator of human behavior. Part of these social norms are manners and polite behavior. These act as a sort of protocol for human interaction.
This all a long way of saying that even though you may think saying "please" is condescending or implies a favor, you are almost certainly in a vanishingly small minority and this will not serve you well socially.
I am trying to give you some insight into why you struggle with social interactions and often rub people the wrong way without intending to or even realizing it. My strong recommendation is you be polite even when you think you don't need to be.
If you put it at the end it reads (to me, at least!) more like a polite request:
"Could you align these variable definitions so they're easier to read please?"
[I think] variable definitions should be aligned so they're easier to read.
Maybe "please align the variable definitions."
When the reviewer prescribes a solution, it requires the author to push back if they want to solve the issue differently. For example, if they noticed they could delete the variable rather than align it.
Though, I'm used to code reviews where the author and reviewer are both professionals and equals. I suppose if the author is an unknown person submitting a patch, you might want to rely less on their professional judgement.
Maybe we just can't help but be offended by nitpicking words because we're all a little insecure from imposter's syndrome, I dunno.
I often go with "I would align these variable definitions so they’re easier to read", to indicate it's a suggestion, but in the end it's up to you.
[0] https://www.merriam-webster.com/words-at-play/question-mark-...
Less flippantly (but still true), knowing that there's at least two ways to interpret something and that at least one of them may be humorous helps me remember not to assume bad intent when I hear or read something I don't like. Often it's just their clumsiness or my over-sensitivity.
On the other hand, perhaps it also helps me hear more of the slights that are in there too! You can't win 'em all.
nit: align variable definitions for better readability
If it's a suggestion or new style that I think we should adopt moving forward, I'd say:
Do you think we should align these variable definitions? I think it improves readability.
Then, ideally I'd leave a non-blocking task for them so they can either do it or not but it's not holding up submission whichever way it goes.
Also, I'd likely raise a ticket against a style guide or project-level autoformatter for that class of suggestion, otherwise it just adds noise to every PR.
One way I write around it is to frame discussion in terms of "we" and not "you." I think "we" reinforces the collective ownership of the code and de-emphasizes the idea of the review being me vs. you.
I wrote more about this technique a couple years ago:
There's actually differences between Australia and England in the utility of "please". In England, fines get paid faster and more reliably if you say "Please pay now". In Australia, they get paid faster and more reliably if you say "Pay now". If I got a fine that said "please pay now" I'd genuinely think I was freely allowed to not pay now if I didn't want to.
And when I was over there, it was really jarring/humorous where there were all these official signs saying "please do this, please don't do that", especially the ones that said "please don't do that, penalty 50 pounds".
So obviously this is regional. I don't really expect anyone to know who they're code reviewing for, but everyone should be aware that there's nothing universal in the interpretation of "please".
For example:
> "What do you think about changing these variable definitions so they're easier to read"?
This gives them a chance to reply and even disagree with what you're saying, because maybe your premise is actually misguided.
In my opinion, if new code has nothing objectively wrong with it(no security flaws, no unacceptable design patterns, no foreseen bugs, etc.), then the author has the prerogative to merge the code. Reviews shouldn't go on in perpetuity because different reviewers would all have written something differently than the author submitting the code. I've experienced this phenomenon at every company I've worked for at one point or another, and it's basically a way for Waterfall to sneak its way back in to an Agile process. (not that I'm an advocate of Agile) It doesn't happen too often, but I find it more frustrating than just about anything else.
Beyond some reasonable adjustments to be made in the first wave or 2 of review comments, any other non-critical changes should be separated into tickets for the next sprint or simply be labeled as DIY or "do it yourself". If you want something done, but there's no actual standard for doing it on your team, then ordering people to write things your way via code review is really just a form of backseat coding.
My code is never perfect, but that doesn't mean I want to change variable names, swap out libraries, or use different approaches merely because someone else prefers them that way. You think the variables should have different names? Wonderful! Go do it yourself. I'm not trying to be rude, but I've got other shit to do, and the software already works beautifully.
I've actually been experiencing this myself, as I've recently started a job with code reviews, and it's been driving me crazy.
> In my opinion, if new code has nothing objectively wrong with it(no security flaws, no unacceptable design patterns, no foreseen bugs, etc.), then the author has the prerogative to merge the code.
One of my biggest personal issues currently is that I have a fear of hitting the GO button, doing one more look-over just in case. I suppose it's just a lack of confidence in my own abilities, or maybe being anxious that my code isn't up to the standards of everyone else and the code base as a whole.
It's also harder to ask people to spend half a day doing extra work on a whim in person.
Many times when someone asks me to change something without pointing to a previously agreed-on coding standard that all team members approved before, I just call it bullshit and let the PR sit there indefinitely while I get to my next task. If someone is interested why the PR is not released yet, I always point out that some people wanted to have changes but they were lazy to make those changes themselves even though the code base was accessible for the criticizer too. And I never fail to mention that I am already working on something else so going back is just the waste of my time and my time is as precious as the criticizer's.
Of course I am not this arrogant when there are agreed-on coding standards and the reviewer points out mistakes based on those standards (but that almost never happens). I am more than happy to make changes in that case.
So to sum it up: pull requests have become the prefect tool for workplace bullying.
Second, not all team norms and guidelines can practically be discussed in advance. It is much better for a reviewer to give a sincere and detailed review than to leave out useful feedback.
> not all team norms and guidelines can practically be discussed in advance
I would suggest that you’ll be more successful if you grow a spine and try to tackle the fundamental issue first: having norms and guidelines. If that's your attitude to programming your professionalism is very low.
You ignored the most important thing in a successful development team's life and now you are suggesting other people how to become more successful? Ridiculous.
Your comment was rude; many people would ignore it.
In the past (and occasionally even recently) I’ve been rude, arrogant, or not valued people properly. It didn’t get me very far. This is the reason I’m writing despite your tone —- because I want to help you see more broadly about what is important.
If you disagree, fine, but open your mind to the possibility that you should revisit this topic later with more experience and perspective. Defensive, aggressive, and ad-hominem attacks are not useful and simply make you look worse.
Being right is not sufficient. Life and work are bigger than that.
If not me (someone you don’t know), I sincerely hope you ask several people you trust for feedback on this topic. I think you might be surprised by what you can learn.
Your comment also makes unwarranted and illogical assumptions. For example, I never said that having norms and guidelines is a bad thing. I only said that there will inevitably be areas that are not covered in advance.
Most importantly, you have a choice in how you act. Aspire to be both correct and kind. This will get you farther.
Good luck.
Just so you know I manage 6 people right now and we are doing quite well. The sort of yours would not be welcome for sure. People who cannot think out of the box will always be boring and predictable.
If you cannot imagine a world where code reviews don't help at all it shows your lack of experience. Work 10 more years in 8 different companies as I did and you will quickly learn that what you say has no grounding.
Being correct and kind is cool. But it won't get you anywhere.
Also, your preferred approach is to passive aggressively let the PR sit until external pressures build enough to force it through.
Do I have that right? If so, I personally find that to be a deeply problematic approach. Though I can imagine situations where it would be defensible and even optimal -- but those situations would involve working with idiots and/or control freaks. In which case my preferred approach would be to find a different place to work.
Does your team have norms around code quality? How are these maintained?
What happens when a commit needs improvement? Does the team discuss it somehow? Does someone fix it with a follow up commit?
Code review is very bad at catching bugs (static analysis and tests do that). If all your code review does is attempt to catch bugs, you're just wasting time.
I'm not advocating for flimsy or convoluted code, yet I don't advocate for perfection. If a reasonable person can understand the code in front of them, and there's nothing objectively wrong with the code, then there should be a clear point where the review is considered done, the code is merged, and any further notes from the review are used to generate new tickets that may get handled by anyone on the team.
This doesn't really matter. Yes, there will always be some level of disagreement. But you aren't writing code in isolation, and it is your job to write code that your team can maintain. If I, a reviewer (who is presumably well versed in the language in question) cannot understand what a block of code is doing without expending significant mental energy, that is a red flag.
I'm talking about refactorings on the orders of 10s of lines of code, not changes to design patterns of architectures, those should be decided much earlier (and in general PRs should be small enough that you aren't forced to redo a design as part of a code review).
> If a reasonable person can understand the code in front of them, and there's nothing objectively wrong with the code,
And this is where we disagree. A reasonable person being able to understand the code is only the starting point. Code should only be merged when, to a good approximation, you're comfortable with anyone on your team (or not on your team, consider onboarding) being able to understand the code quickly.[0]
One pet peeve of mine in this regard is loops that break or continue, especially those that are nested. They're often easy to write, and sure I can figure out what's going on eventually, but it's not easy to maintain, and there are straightforward, mostly mechanical changes that can make things easier to follow:
a loop with a break is almost always an attempt to find something in a container. If you're continuing, it means you've got a function that finds and then does something with the found object. Split it in two. Have a function that loops through and returns the result, and then a second function that processes that result.
A loop with a continue can almost always be refactored into a filter (in practice, I am not saying you should be using map/filter everywhere) by reversing the conditional.
There are exceptions to this: early breaks/guards (sometimes), "returning" by reference in C, and sometimes building state machines can be done most clearly with continues. But those are fairly context specific (and they aren't the context I work in, for the most part), so can be called out explicitly. That's the kind of thing I look at.
[0]: Another way of thinking about this or justifying this: If someone gets paged in the middle of the night because there is some production issue happening, and the trace happens to cross through this chunk of code, will it significantly delay this groggy person, who is neither you nor the author, and who has perhaps never looked at this code before, from quickly isolating the root cause of the issue?
Generally for code review, I find the best strategy isn't "this could be better", and _definitely_ not "I would have done this differently". Currently, I think it's "this gets the job done (is tested, passes acceptance tests), isn't overly clever, and looks like most of our other code". Anything more than that should either be a larger refactoring/rearchitecting job, or threatens to quagmire you in a tarpit of subjectivity.
To be clear, I am not suggesting anonymous functions, nor am I suggesting using the map/filter constructs. I'm suggesting that
def my_process_seq(seq)
for obj in seq:
if meets(obj, cond):
break
return process(obj)
def get_obj(seq):
for obj in seq:
if meets(obj, cond):
return obj
process(get_obj(seq))
> not "I would have done this differently"
I agree.
Also for me the review has easy "done" definition. It ends either in "code approved" or "changes requested" state. The first one leads to merge, the other one to further work or discussion on the code.
Once you met those then, yes, your code should go through.
I think the key here is that it should be possible for reviews to mention improvements while not treating them as blocking the commit. When I was reviewing code I would often say this explicitly: "You don't have to do this, but..." Or, I would prefix some comments with "nit: ".
If you're always pushing people to say less in code reviews and they listen, it can turn into a rubber-stamp process. Reviewers need positive feedback too!
Then, the author can implement the tasks they find reasonable and ack the rest and not be blocked on submitting by a bunch of back-and-forth over style details.
Just getting to code reviews at all is a huge win for a team, every process after that (including "definition of done" for reviews as well as tickets, merging without signoff (I do it for small trivial things) and the post's "be nicer" guidelines) should be discussed as a team in retros. Something that's worked for me (mostly on remote/distributed) for making sure reviews don't stay in limbo is as an author to be a bug and aggressively ask specific individuals for a review, even going so far as to schedule time to hop on a video screen sharing call if some background context and walkthrough is helpful. But the team needs to be accepting of this, otherwise again you're being difficult...
At a team level another successful practice was for everyone to block off a convenient (not identical) chunk of time once a week to dedicate to making sure you have no pending reviews waiting on you, and also letting you say "please wait until [time] or see if someone else is free" when you're busy and the author bugs you. Sometimes a good review of a large change takes more than a focused hour or two -- it's fine to push back for smaller changes, but if you just block off some time, you can plan for it and just do the big review and then a limbo period will just be at most a week.
Some things, like whitespace, braces, and naming conventions should be obvious.
That's why the best rule of thumb is to stick to style, names, and conventions that are already in use. The review is to keep it consistent.
Not that I don't want to write quality code, but nitpicks in the name of quality are the code review equivalent of a death by a thousand cuts.
- point out strict violations of the team's enshrined code style guidelines
- accept any and all code as-is, so long as there are no "bugs" or performance issues
Which, at that point, why are we even doing PRs? Unit test and lint...
When I receive a Code Review I want to know about the function that reduces my 10 lines to 1. I want someone to notice that my git mistake introduced a bunch of unneeded diff noise. I want to change variable names that are confusing to the first reviewer, because that means they'll be confusing to me when I read the code again next month.
Give me the development "human factors" in the code review. Some of that will be subjective and that's ok.
The strong reactions are due to pedants who block the source base doorway and pick at everything going by. It can sink a project. So folks push back, say things like "Only comment if its a real bug!" and so on.
There's lots of middle ground in a company source process. For instance, point out real bugs if present. But for anything else, comment and approve, respecting the contributing Engineer to do what they can in the time available.
But the tips in this article work for subjective comments too. Rephrasing comments as questions is nearly always applicable.
I make a lot less code review comments than I used to because I've seen too many bugs introduced by fairly innocuous looking code review changes and the amount of bikeshedding that can go into code reviews comments that will likely never materially effect the application.
I think that is actually a good bar for a CR comment. Issue of maintainability affect the application. Bugs affect the application. A lack of general good software architecture has long term effects on the application.
All other comments I either tag on an "as an option, you could..." or force myself to shut down the "that's not how I would have done it!" voice in my head and just leave no comment.
On the receiving end, few things are more obnoxious than "deck chair rearranging" comments which have no bearing on performance, maintainability, style, or correctness, but just boil down to you satisfying the benign preferences of whoever is looking at your code.
A lot of this advice could be boiled down to "Would you make this comment on reddit, or would you say it to your colleague's face?"
The only thing I would add is:
>If a submission is too large to be reasonably reviewed, it is okay to let the submitter know right away. Keep moving forward.
Doesn't seem right to me - or atleast the focus really should be making sure that no one submits code for review that's too big for review, and that means early feedback about a feature getting too complex and breaking it down. It's much easier to go into a problem saying "I'm going to do X, it's going to be huge, so I'll do A then B then C and submit them separately". If a submission is too big to reivew it's probably non-trivial to go back and break up so that can be quite annoying.
I even found the relationship non-linear. I.e. if you can break up a big change into smaller changes that have more overall code, you can still probably get those reviewed quicker.
I probably would have learned the lesson faster if reviewers had just rejected the changes for being too big, or at least a "next time you could have split it this way".
1. Sit at the desk of the person and pair-code review. Helps avoid basic misunderstadnings, and also is more private that a permanent written record where people may feel defensive about their choices. Also it's easier for a lot of people to be rude through the internet.
2. Write in small notes if any must-address concerns arise and are agreed-upon (e.g. "Security?", "duplicates function x")
If your devs can't interact face-to-face, then I think you've got major cultural problems or hired the wrong devs.
Introducing any form of friction makes reviews less desirable for everyone involved.
Code reviews accomplish a few things:
- They are primarily a discussion about code changes. ("Why did you do this?"
- They help onboard new members of the team, both in terms of how to work most effectively with the code, and with style. (No style guide is 100% exhaustive, "We normally name variables for that fooBar, not barFoo.")
- They help team members learn from each other. ("That's a cool new function in the new version of XYZ library!")
- They help enforce good engineering practice. ("I'm not merging that until you write tests.")
- They help make code more readable. ("Please add a comment here, and rename that method to DoSomething.")
- They also quickly identify people who shouldn't be part of the team. Everyone sees that someone moves slowly in a pull request, or that someone resists cleaning up sloppy code.
The thing with people who resist cleaning up their sloppy code is that it shows to management. This happens when everyone else in the team gets their pull requests merged quickly, and one person's pull requests pile up with lots of minor comments.
I am frequently surprised by how lacking many technical professionals are in EQ/soft skills/civility -often I observe people who apparently believe that their skill is such that they don’t need to treat their peers with respect. This isn’t anomalous; it’s widespread.
[Conjecture/opinion] I believe that many people who enter software development and related fields are high on the autism scale- eg aspergers, etc., and that many such people are unaware of how badly that they’re treating others. It’s not “common sense” for many.
I personally think that terseness is actually polite since it wastes a lot less time for both sides.
One reason is that the maintenance burden falls on more than just the author, so there needs to be some consideration on whether it is maintainable and whether the people involved are willing and able to maintain it.
Particularly if the change adds a public API or behavior, it may have to be maintained for the life of the codebase.
Also, some projects have very high reliability expectations and you have to assume that code is broken until proven otherwise. I won't merge code until the author has demonstrated via tests, comments and clearly-written code that the change does as expected and doesn't introduce potential issues.
The attitude in the article isn't particularly conducive to the long-term health and quality of a project, since you get a bunch of authors ramming through their changes based on their agenda and end up with an inconsistent mess that nobody wants to maintain.
It took me a while to realise that the same message can be delivered in a far nicer manner, which generally leads to a lot less conflict.
Now it almost comes naturally to write nice comments that deliver the same message, namely: the code could be improved
Careful with this one. In my experience, you aren't fooling anyone when you ask someone a question to which you yourself know the answer. It can quickly come off as condescending.
One helpful practice that we have is that we make a distinction between blocking and non-blocking comments. Nonblocking comments are desired changes, but aren't required by the submitter to fix (formatting, code simplification, naming, etc). This steers people in the right direction without preventing them from getting work done. Blocking changes are critical things which must be resolved before the code gets merged (security risks, performance concerns, complexity concerns).
And of course, every comment should be followed with a suggestion for how to resolve it (even if it's meeting in person). "This is bad" obviously doesn't fly.
I haven't been in too many code reviews, but my experience is that it is often taken personally if developers aren't friends with each other. I vastly prefer unit and integration tests to measure code quality.
I can also understand the reluctance of maintainers to include code of a different style. Here the tips from the article probably help, but I think it happens often enough that code is rejected for non-technical issues.
I'm not aware of automated ways to validate architectural soundness and other abstract features.
For FOSS at least, ultimately maintainers and authors are not beholden to anyone who wants to submit code, it's perfectly ok to say NO even for political or emotional reasons... so if not in the direct context of a PR then where?
I think heated and unconstructive PRs probably arise as a result of pushy and entitled submitters, after a certain point authors will inevitably get fed up and abandon diplomatic approaches.
Linux might be incisive but he's often right and it's usually about an impactful issue. Some other people are just gratuitously more rude and/or obnoxious.
Usually, there are multiple ways to implement something and that there is no clear choice which is better. Trying to make into a black/white problem, is not the best way to deal with the inherently grayness of software engineering. Make a fuss about coding standards, for example, is often not productive.
Once you have a coding standard, then the reviewer needs to differentiate between opinion and fact. If you don't like the variable name but it conforms to the coding standard then it's not a valid review comment. The coding standard should give some guidance as to what a variable name or function name should be.
If there's a bug or if there are potential for issues then that's what I focus on.
- Not having the required soft skills to do it. - Cultural differences. Because, What is considered offensive, rude, or negative in many cultures, might be just direct in some cultures. But I do believe cultural awareness is also a soft skill. - Having a bad mentor/peer/environment. Many junior developers write reviews with the same style as they receive reviews.
- first
- second
instead of
-first -second
https://ibb.co/jLtZTFS - image shows editor pre-submission ;o)
HN should use [limited] markdown or something IMO.
We absolutely must consider the non-technical aspects of a change, and it behooves us to consider them before other aspects.
Changes are often driven by new requirements. If we approve the change, we are approving the adoption of those requirements.
There isn't always a technical reason for such an adoption other than "someone wants it that way".
Based on this article, I mustn't reject, for instance, GPL-ed code being added to a BSD-licensed program. There is no technical argument against it. We can just switch everything to the GPL license and march forward; what's the problem? Can we put religion aside and just discuss the code?
There also no technical reason why a compiler shouldn't have a --tetris option for playing a little game on the text console. No wait, I didn't say anything about text; we can just link in SDL and have it graphical! Anyway, can we just discuss the best way to do this, and not emotional debates about whether or not to do it?
So, "someone wants it that way"? Whooptee doo, so effin' what? Let them fork the code.
"Oh you spent 2 weeks figuring this solution out? Well here's my 2 minutes of naïve ideas..."
And, of course, commit messages.
The second most infuriating one was series where I had to rewrite code because rewiever did not knew syntax I used amd claimed it is difficult to read. Three weeks later he was forcing me to use that same syntax - because he found some blog where he learned it so it became necessary. Like what the hell.
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.
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.
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?
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...
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.
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'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).
(or possibly to avoid being sheldon in the big bang theory)