Congrats to the author for realizing that confidence doesn't equate skill. This isn't being a pink unicorn, it's simply humility.
The real danger of confusing the two is that real skill dulls if you let overconfidence take over, since the very thought of being "better than others" prevents you from accepting that maybe there are things that you don't know or haven't considered.
For example, is "destroying" a coworker's commit in a code review really really helping? Did the person perhaps forget to consider that hiring to refill the position of a fired developer actually costs physical money (as opposed to time spent on coaching?) or that perhaps a lot of time is being spent on nitpicks without bottom-line value or that tip-toeing around egomaniacs can stunt autonomy and therefore creativity? This is not merely a matter of whether you're lulling yourself into some false sense of superiority/duty/whatever; thinking in terms of bigger and bigger pictures is an actual skill that one needs to develop when going up the career ladder.
Yup! I have been very fortunate to work with extremely kind and empathetic senior engineers in the past. They have done wonderful jobs at using the code review process to teach instead of tear anyone down. I am always shocked at how many programmers become so aggressive about development. It might be silly, but I think the xkcd comic Ten Thousand is a great mantra: https://xkcd.com/1053/
Teaching people cool new ways to code should be fun, not an exercise in showing everyone the size of your ego.
There are many ways to solve any problem. If I reviewed every PR with the standard "is this exactly how I would have done it", I too would be destroying every PR I review. And I'm not even that good at coding - I'm the type of generalist who cares about things like the business beyond the code base.
What really matters is whether the code is solid and whether it will actually cause problems. Just because a PR isn't character-for-character how I'd write it doesn't mean it's bad. Based on the author's experience with the fired employee's replacement being just as bad, my guess is he was over defining goodness and raising the bar too high.
Bad PR to me means: bad runtime complexity, bad formatting, bad security, bad memory use, bad understandability. If it meets all those bars, which for a crud app is usually easy, then it should probably pass. I'll sometimes leave a "I would have done _x_ differently" comment on matters of taste _if I think it will help the other developer_.
> If I reviewed every PR with the standard "is this exactly how I would have done it",
This, too many people think their way is the only way. It only takes one cancelled project or one company going bankrupt (and 100,000s loc going "poof") to understand this work we do, really doesn't matter all that much in the end.
I've spent over 6 years working on 5+ legacy projects that had millions of lines of code and maintenance teams 30 times smaller than the army of contractors who developed them.
And I've learned all the small nit-pick maintainability issues made no difference. Whether or not there was a style guidelines, and whether or not it was enforced made little difference.
Things that really made a big difference were giant bone-headed architectural decisions like "just save persist this data as serialized text", "lets write all the business logic in stored procedures", "building the software on a terrible platform".
And having some type of identifiable domain model helped a little too.
It is a really humbling thought that every single one of our entire life's work as developers is one unfortunate astronomical event away from vanishing as though it never existed. Kind of puts it all into perspective really.
Of course there are thresholds for competency. But if you find yourself having to destroy every PR from a developer you're probably toxic & insufferable.
When you're in these situations, you are faced with a challenge which isn't solved by code reviews no matter how illustrative, nice, or demeaning they are.
It's first solved by identifying what the cause is of their poor performance. Are they in over their head? Are they having personal problems which are distracting them? Are they experiencing burnout?
I've heard people say that they enjoy code reviews because it gives them latitude to be ruthless. After all, it's for the benefit of the business to not allow suboptimal code through, and it's just code. But frankly, I think that's a lame excuse and the easy way out.
There are people behind code, and while the code might suck, the people don't. Obviously, that doesn't mean you should let it through, but it does mean you should be at least professional, and preferably compassionate in how you review it.
Humility goes a long way in life. Anecdotally, it's definitely something a lot of software developers and systems engineers should learn how to practice.
> And if you tell me that you haven’t had this feeling ever, then you’re lying. Tell me about higher goals, training rookies and all that — I know you’re simply too full of themselves. And if you try to tell me that you learned to defeat that feeling (however it manifests in you), then I must be a pink unicorn.
That line also annoyed me. I think this person has a lot more self reflection to do about their insecurities. I am glad he has begun to recognize there are some issues and I hope he is able to continue to improve himself.
I'd like to point out that every one of the "correct" examples is longer, sometimes much longer, that the "wrong" example preceding it.
I've found that I usually start with the "correct" way and end up regressing to the "wrong" way as more and more work piles up, stress builds and deadlines approach. I guess the "correct" way only works if the organization actually prioritizes it and gives reviewers the time to do it the "correct" way no matter what stage the project is in.
Nothing makes working at a place more unbearable than having to deal with someone that gives an excessive amount of criticism on working code, at least that's how I feel. I've been in that situation before and it made me never want to submit pull requests. It made me even madder when other developers, whose code was no better than mine but had been at the company longer, received basically no critiques.
Some might say, oh it's hazing, well fuck you because I'm quitting. I'm not putting up with any amount of hazing at a job. If I have to spend 8 hours everyday somewhere I'm for sure not going to spend it with some asshole.
Code reviews can also have the opposite effect, I sometimes ask mentees of mine to review PRs of mine. Not necessarily to get their feedback, but to get their questions and to help transfer knowledge.
I used to do that when I started working on open-source as well, doing code-reviews to learn the code base (often I would not submit the review to not create noise)
> Honestly, I’m not sure all of them are experienced enough to give a useful critique
I wouldn't let this be a stumbling block to asking for more review. I think it's like writing an article or paper - once you've invested a lot of time into the thing it gets harder to see the warts. Having another set of eyes can really help catch that stuff, even if they aren't at the same level of expertise as you. Back in my research days the department I was in had a person who's job was basically reading research papers and providing feedback - in this case she usually focused on the paper structure, clarity of phrasing, and of course grammar - she was an expert in writing not in CS. Yet looking back a drafts of papers before and after she had given input shows a massive improvement, even if the technical content was unchanged. Similarly a smart intern can be a great source of input for making my code clearer and better, even if the algorithm doesn't fundamentally change.
Also, if like me you are prone to falling into ego traps, inexperienced folks can bypass the ego's defenses. A simple "hey why did you do it $complicated_way instead of $simple_way?" can hit a lot harder than "hey make this $simple_way" from a mentor. I think it's because I've invested time into teaching the intern and when they ask a good question like that my ego gets a boost from "I taught that kid well" to balance out the "OMG YOU THINK I'M TERRIBLE AT THIS LETS FIGHT" response. (It may also be tempered because I tend to view the intern's questions as genuine requests for input, so I start thinking of the explanation and realize $simple_way will in fact work just fine.)
I don't think it's something you should have to ask for. I mean if you went to a nightclub and the doorman let you in, would you say "Hey, aren't my shoes a little out of fashion?". To beat the analogy to death, I suppose if I was walking into a nightclub naked, on fire and carrying a bomb, I'd be a bit worried about the establishment of the doorman didn't refuse me.
So: is your code naked, on fire and carrying a bomb?
IME, if the tests don't fail the first time you run the code, it says that the tests aren't actually hitting the new behavior. (Maaaaybe 1 in 100 times the code is actually right the first time.)
Likewise, if there's no comments on a pull request, it hasn't been read...
I'd have agreed once, but at some point in my life I realized most of my code really was working right the first time, even messy javascript. Anyone else have this experience?
Super critical code review culture can also end up with balls of mud. The two are not exclusive. Example is a team I was on that would critique the hell out of syntax and grammar in javadocs, formatting of annotations arguments, stuff like that, fine. But never look outside the source file for how any of it fit together. No design planning cause "code reviews". End result was tons of duplicated code everywhere.
I've submitted code that was reviewed like that, and it was super frustrating. Stuff like, "your javadoc uses invalid markup", when submitting code to a project that has almost zero javadoc to begin with.
I mean, sure it doesn't render well as HTML, but its readable with the code and the rest of the project has none so...
Ask for some time to walk through what you've done with someone.
I've been on the receiving end of enough toxic code reviews to develop an aversion to do them. So when asked to do them, I really just ask, did you test it?, do you want to demo it to me?, then approve it.
When people demo to me, I can see their pain points, because they always point them out. "Oh yeah, I was having trouble getting this to work, as you can see, it's still a little slow" and it gives them an opportunity to ask for advice or merely to vent.
So if you want good, honest feedback, a demo is the way to go. Honestly, it's hard to really grok what a bit of code is doing just by reading it. And who has time to pull code and run through it solo when doing a review?
I think part of the problem with the bullying described in the article and the "approve everything" approach starts with the framing of code reviews.
Instead of "please write this code, submit a pull request, and I'll tell you what you did wrong," I have recently been saying to new coders "take a shot at this problem, and when you've got something working let's get together and refine it, there is some context it will take time to learn so we'll probably need to make a lot of changes or even rewrite it before release."
That way they know what's coming (a lot of changes), they know why it isn't a sign they're inadequate, and it's more about working together than passing judgment.
It has also helped reduce the massive delays while junior developers agonize over making something perfect.
I did it for three decades (3.5?) and the only code reviews were with friends of mine who worked at multiple companies together. The whole point of the review was to find outright errors, not to optimize. Scarcely any ego involvement.
It worked out fine I think and, mind you, it used to be a lot harder to push out a fix. A lot harder.
Yeah, maybe I should add, we would hang out in the hall and whiteboard our ideas, workflows before we went back into our respective cubicles/offices and started coding.
I think the issue is when a code review is “do this”. Especially when it’s small things like formatting, and I have to wait a day for the PR to be approved (or they catch another formatting issue).
It’s better when code reviews are suggestions, the reviewer doesn’t nitpick (or at least fixes that stuff themselves), and small things like formatting are enforced by a linter.
It might mean you're doing it basically correct, or it might mean you're touching parts that people don't care about much. The second is probably more common.
(It might also mean your team doesn't care about anything any more, in which case look for a better opportunity, unless you want to just cruise along for a while for whatever reason, in which case turn into a bare minimum performer like the rest of your team).
I'm a senior engineer in a known company and my target is always to leave no comments on a patch. I only give question-comments if I absolutely must know something before merging, or please-fix comments if something is definitely wrong and I can prove it. Also, if it's a part of the system that poses little risk (for example, if it breaks I can assign the bug back to the author and patiently wait for a fix with no serious damage) my tolerance for bugs greatly increases. The last part I'm not too proud of but frankly I don't have time to review everything. A "fix" for the last part would be to find a lower rank reviewer-buddy who has more time to share knowledge.
Thank you! I agree with so much in this, it really strikes the right balance of clean code vs productivity.
One of my peeves is a review that points out a bug/incorrectness without offering proof, or suggesting the fix/correct way. The reviewer might observe a code smell, but it's so much better dealt with a question-comment, e.g. "did you try?" vs "this looks wrong".
I generally aim to make every comment a question ("can we..."), and not make assumptions that the author didn't think of something already. Unless I just stating an objective fact that is clearly unknown e.g. "this duplicates library function x...".
Yeah the general style of putting comments into questions is much better.
But that's not exactly what I meant with question-comments, I sometimes genuinely don't know why are they doing something like this, or have to ask for additional testing/evaluation/analysis/profiling results before I can decide that it's most likely not going to blow anything up. And as long as there's low probability of damage (or rather low expected value of damage) I am happy with the change :)
What really helps is a style guide. It shuts most of those pointless style nitpicks, God I hate them.
Conversely, nothing makes working at a place more unbearable for me than working with people that over-engineer everything. These are the types of PRs that tend to get the most feedback, and also the type of person that complains most when receiving this feedback.
Not only that, but the title of the post is, "I ruin developers’ lives with my code reviews and I'm sorry".
This person is so confident in himself, that he thinks his code review takedowns are so brutal, yet effective, that he is ruining lives. It almost comes off as "I'm so handsome that I ruin people's lives with my looks and I'm sorry."
And then there is this...
"I was mad that, while I spent my nights learning F#, my daughter started calling everyone around “fathers”. And this guy, instead of getting better at his job, went home to his children. And I wanted to punish him."
I had the opposite reading. It feels obvious he's creating a caricature, some of which is exaggerated, some of which is truthful of not just him, but other developers.
The recounting of "bullying"/hazing on forums definitely resonates as well, and he's just remarking that he'd become what he'd hated.
This is more of a reflective piece rather than a glorification of his behavior.
>> My personality today isn’t my disease. It’s a disease of the whole industry... just stop being that. It’s quite easy, actually.
I've had the same thing the other way, where I feel that I usually give kind and fair code reviews (no nitpicking, etc) but have had former colleagues that get genuinely upset and push back really hard when I make any sort of comment on their PR.
Because it creates technical debt, that's why people get frustrated with shit code. They just aren't properly equipped how to convey/teach/train/fix it without coming across like an asshole.
That is exactly why many harshly critique code. But I've never found their versions to be any less full of tech-debt. Usually it comes down to personal style. "Oh Brenda hates if/else, better convert it to ternary before I submit it. But Oh No! Bob is reviewing it instead! He hates ternary and always wants if/else! I'm doomed..."
Sounds like that team should define a style guide and stick to it. Either Brenda or Bob is going to be disappointed for a bit, but the code will be more consistent in the end.
This is why automatic code formatters such as Prettier -- are absolutely genius for teams. (It doesn't work for your particular example of ternary vs. if/else but the point still holds.)
I got annoyed by people leaving "philosophical" or "style" type comments in code reviews - i.e. nothing related to functionality or performance but just different ideas about how things could be written without obvious advantages. Then I realized I could just write something like "Thanks for the feedback. I've addressed the points that I feel are critical, the remaining comments can be addressed in future if necessary". Then I'd just ignore the worthless comments.
My mental model is that some people feel like they must comment on a code review to show that they've done something. They don't actually care, but just want to write a comment or two and you can shrug those off.
If following this approach you do need to be careful to only ignore genuinely useless comments though. Often a good way to tell is just to chat with the reviewer. That has the added benefit of the reviewer learning that useless comments will create more for them because you'll be by to chat about their useless comments.
While I think you're taking the right approach for getting the work done, maybe the comments aren't entirely worthless? In some cases, those are the best comments I've gotten when having someone review my code, even if I didn't make a change because of them. They're something for me to think about and either incorporate or not in some way in later work.
In other words, I think it's correct to not consider every comment to require a change unless it really needs it, but that doesn't mean a comment that doesn't require a change for the current work should necessarily be ignored because of that.
i have been on an ever onward journey to try to find the balance:
- is a developer making the same set of mistakes? send them information on a better strategy to try overall
- how bad is this mistake? Is it just something that I know how to do better, or legitimately a broken way of thinking?
- what is worse, a slightly inefficient or unpretty algorithm, OR someone researching the problem / solution
Point is, 90% of my comments were meaningless. I really comment when things look wrong -- Did they misunderstand the intent of the ticket, did they verify everything with product because this looks weird, did they check the edge cases, did they forget about this other behavior, etc. All that I just mentioned is the real meat-and-potatoes. Code-fu is important, but less important than the other stuff. And if you are a calm, trusted, and mentoring team member, those same people will come to you to up their code-fu voluntarily.
I had an incident many years ago where I found glaring design flaws in a much more senior developer's code, and, overcome with some feeling of superiority, left a million nitpicky comments and a snide summary, then posted on our team chat "left a review". Finally, I was getting back at all the senior developers who had torn apart my code.
I later realized my comments were mean and I apologized. While you could still potentially call it constructive criticism, there is a difference between mean criticism and polite criticism.
I do think thorough feedback is important, that's how I've learned the most after all. But typically now if I I'm going to write more than 5 comments, I meet with the other developer in person instead. That way the code problems are solved faster and it avoids any sort of public humiliation.
> If a guy brings me his code, and it has mistakes, it brings insane pleasure from how smart I feel [...] And if you tell me that you haven’t had this feeling ever, then you’re lying. Tell me about higher goals, training rookies and all that — I know you’re simply too full of themselves. And if you try to tell me that you learned to defeat that feeling (however it manifests in you), then I must be a pink unicorn.
I really can't relate at all. I feel the opposite. I often feel angry (I realize this is not a GOOD response either!) and always feel sad. I don't want to have to do more work. I don't want a reminder that my previous feedback didn't click with them.
I'm constantly looking for ways to do less tedious work, through automation and such, and code review for bad code is the worst sort of tedious work. I want to learn things, but code reviewing bad code doesn't teach me anything - I want to work with people who can teach me things when they review my code just as often as the reverse.
My takeaway from that has been to try to intercept people earlier in the planning and designing and start-of-coding process. If a bunch of bad code gets to code review, someone failed at explaining what this thing needed to do and how it should be done beforehand. The article claims this is always just itself a confrontational form of argument, but... it doesn't have to be. Stop being an asshole, and find non-assholes to work with.
Yea this response seems like something a crazy person would say. The first thing I tell people I am training or mentoring is “try to separate your ego from the computer”. I think that’s good advice and practice for trying to learn anything, but especially in computer work. There’s just so many situations where things should be working but they arnt because of some niche random issue. If you involve your ego in this work you will be riding an emotional rollercoaster and you will be less efficient and less effective.
It is possible that the person is accidentally projecting their internal state as a model of how other people feel when they do code review. It's easy to accidentally fall into that trap. I've probably made that mistake before.
The truth is that people's emotions can be a lot more different from yours than people may be willing to imagine.
No one is perfect. But not everyone derives 'insane pleasure' from criticizing, either. And if you do, that may not be obvious.
I agree with the 'stop being an asshole' line, but to do that we need to help people realize when their qualia is way out of the ordinary. If you think everyone else is power-tripping, too, then will you think you're really being an asshole, or are you just doing what everyone else does?
It's scary to think that you might have something else you think everyone else does, but is really just you accidentally being oblivious. Speaking for myself, I'm going to try to be more mindful of this trap in the future.
> I was mad that, while I spent my nights learning F#, my daughter started calling everyone around “fathers”. And this guy, instead of getting better at his job, went home to his children. And I wanted to punish him.
This reads like American Psycho. I can't believe this guy is for real. You have nothing but your job, so - at your job - you try to make everyone you work with miserable? What? Why?
Have you considered trying to make your life enjoyable ever?
Same here. For me, code review is a good time to share knowledge and help other people. If a problem often comes up, I usually try to see if a tool can solve this. I'll admit that it can be really hard to know what level of tolerence people have for nitpicks and things like that, and at which point you're starting to sacrifice team spirit/people's feeling for small things.
That's why I love pipeline integrated scans that point out issues like excessive cognitive overhead (big fat if/else waterfall with loops everywhere) or "code smells" so the team doesn't get bogged down in munitia during code reviews. If someone needs help getting a piece of code tested to 100% or resolving a code smell they can reach out or bring it up in a standup.
The other thing to keep in mind is that most code is fine.
When one company buys another the quality of the code is a calculation in the sales price. But it's a small one, and it takes a real dumpster fire to move that needle.
I have worked with people like the author. They suck. They destroy entire teams and often times the feedback is 99% pedantic personal preferences because why would they do something useful like setup the linter and propose the silly things they always nit pick.
These people don’t last long and I have gotten pretty good at screening out this personality in interviews after having it cause huge issues at two different places I worked.
I'm curious to know if you or anyone else reading this has run into the opposite problem. Or maybe I'm just "that guy" although I am reasonably certain that I am not.
As a rule, so far throughout my 5 year career as a full stack web developer, my coworkers have neither understood or cared about what clean code entails.
I don't berate, I don't criticize, I've taken to mentioning it once and then doing my utmost to never mention it again in an attempt to avoid labeling myself as the odd one out.
Granted, I've turned down a handful of much higher paying (and presumably higher quality teams) for various reasons. At first, it was because I was a bootcamper and high paying jobs are difficult to pick up out of the gate without the appropriate piece of paper. Since then, I've worked for various organizations on a consulting basis, and most recently landed at an early stage YC startup. Pay isn't great, but I have aspirations to start my own startup, so I'm cool with the pay cut for now in return for the opportunity to learn the ropes, business wise. Founder knows what he's doing at least.
But the devs, man. They just don't care. 500-1500 line React function components are the norm. No standardized way for retrieving data from the back-end. I dread assignments that require me to modify or otherwise integrate with previously written code, because the best way I can come up with to figure out what the fuck is going on is to rewrite it sanely. I'm being a little hyperbolic, I do my best to avoid rewriting wherever possible, but I'm not mis-characterizing things by much.
It's honestly depressing, especially considering I've had multiple other teams offer triple what I'm making right now, but neither of them were pre-seed startup SAAS companies which this one is, so here I stay for the time being.
Invariably, I garner a reputation as "that guy" on any team I work on, although granted this is only the second multi-dev team I've ever worked on. Still though, it feels terrible to get a bad reputation for writing good code. Founder likes my work ethic and output though, so I guess I've got that going for me.
Apologies if this turned into a bit of a rant, it just started pouring out, ha!
I know of a few startups with relatively 'senior' people who just don't care at all or are seemingly unaware about 'best practices', and will happily write tens of thousands of the most messy code, neglecting standardizing anything or refactoring their code to be more modular or easily understood.
It does seem to be something the big co. interview gauntlet is designed to weed out, whether it's worth it or not. I'm sure it's still relatively common, but probably a lot rarer than at the median startup.
It’s possible there are things you are still learning, or context you are missing.
You sound still early career enough that the years will likely mellow you out and teach you to better appreciate the ups and downs of building things fast.
You know what’s worse than no abstractions? The wrong abstractions.
Not the OP but also a full stack at my five year career mark and your last point was an appreciated reminder.
I dread cleaning up solutions that I designed earlier where with hindsight, I made a lot of wrong assumptions. I still make wrong assumptions today, they are just designed less brittle.
You are right on the value of building things fast.
However one thing I have learnt is more coupled the code is with no or poor incomplete abstractions less likely anyone is going to change it.
In the interest of shipping fast I would rather have some abstraction today rather than none, simply because nobody fixes spaghetti code which is already working.
Building new things start slowing down more and mode and you end up taking weeks for what should have been an afternoon's effort .
There are no right answers, however sometimes wrong abstractions can be marginally better than none.
Oh c'mon graybeard, you can't honestly be defending 1500 line components right now can you? It's not even a matter of abstraction choice, it's a matter of simple hygiene.
You know what's worse than the wrong abstractions? Copy and pasting your inline-styled div with a zIndex of 99,999 for the fifth modal in a row because you can't be assed to write a damned display component in anticipation of the next time you'll need to put a Modal together.
No pushback here, I’m moreso suggesting that length is not intrinsically the enemy. I would much rather be debugging or wrangling a long functional component with duplication than a messed up or poor abstraction that’s leaked out elsewhere in the codebase.
Levels to it all. Keep track of painful modules and have strong intent to fix them. Prioritization is a mf.
> Why not? If the other devs truly don't care, you could take it upon yourself to clean up the code base little by little when you can.
I used to do this simply because it makes the code difficult to read for me to review if for nothing else.
Linters and style check tools only go so far, and bottom line is you have to ship software not write complex and expensive to maintain tooling, so automation beyond a point is not a solution.
The devs ended up relying on me as the their clean up guy for all the code all the time. I also becoming gatekeeper/bottleneck of sorts.
It is used make blame and dissection more difficult as more times than not I had cleaned up the code and the last one or two commits are mine.
Also the time spent in reviews reduce and you tend to miss important things as you have focused on what should be basic hygiene so much.
One of the reasons I contribute a lot less to production code is this. It is a losing battle if most devs don't care about it as much as you .
Even now the occasional pull request I would make would be peppered with such corrections , it is just automatic reflex for me, but it doesn't make that much difference.
Don't do this, people either have to value the cleanliness enough to do it as well, or you have to tolerate what is the common minimum or find another team/organization.
You'll always run into that problem if you care. Most of the time this results in having a clique against you who'll make your life hard, for example with increased bureaucracy.
If the manager is on your side, it's possible but not fun.
It’s certainly possible they don’t care, or they don’t know how to write good code.
Another possible reason could be that they know it’s temporary. A lot of startup code gets rewritten at least once in the first 5 years, there is some art to knowing what code might stick around and be worth the effort to do well, and what you should just bang out fast and move on.
It sounds like it’s possible you haven’t been able to have a conversation with your colleagues about their choices, which might be worth having candidly if you can find the opportunity to do so. If it is ignorance or negligence better to know and decide how you want to proceed if that makes sense.
Couple things about this, because there are levels. First, understand to be a successful engineer you need to be a bit pedantic, so start by forgiving yourself and take a deep breath.
Once you become competent enough to identify clean code, understand that your perception of clean code is colored by your own experience and current mental context. As you learn, you will undoubtedly be aghast at your own code that you wrote at any time before 6 months ago. After you've been at it for a couple decades you will come to the horrifying conclusion that your mental model is always incomplete, the human brain is weak, and it's hard enough to not get caught in the wrong local maxima without facing up to the immense challenge of choosing between global maxima in the incomprehensible problem space where software meets human needs in the hybrid meat/mental/cyberspace ecosystem where they exist and evolve over time.
The other implication is that no two engineers will ever see things the same way. It's not purely about technical strength—a weaker engineer might come up with a better solution given more time to think about it, or due to some other blind spot or skillset gap from the more senior engineer. Also, there are many issues of style and perspective where the important thing is consistency more so than some specific choice. In those cases there's a high risk of bike-shedding, and it leads to a fundamental truth of large scale software development which is that in practice you will never get everyone on the same page. Of course there are controls for this, but unless you have the budget and operational scope constraints of NASA (or perhaps medical device manufacturers) you can't actually enforce this without losing so much development velocity that your company is utterly uncompetitive in the market.
As you move up the seniority and scale ladder to larger projects and architectures, you will realize that you still only have 24 hours in a day, and it doesn't matter how smart or how right you are, the only tool in your built to increase your impact is a simple cliche: "pick your battles". There is absolutely no upside in nitpicking every line of code that could have a conceivable improvement. If you want respect from your colleagues, you need to get good at filtering what is important and what isn't, and only pointing out the most important issues, and doing it as early in the process as possible.
I appreciate the reminder and the helpful tone of your comment in general!
> As you learn, you will undoubtedly be aghast at your own code that you wrote at any time before 6 months ago.
Just checked, and my code from 6 months ago looks identical to what I write at work. My programming style went through a few different phases but its variability has mostly leveled off since I've started writing in a mostly-functional style.
> It's not purely about technical strength
It's simple stuff like putting together a base Modal display component for re-use instead of copy/pasting that div with the inline-styled zIndex of 99,999 for the umpteenth time.
> As you move up the seniority and scale ladder to larger projects and architectures
I hope to side-step a lot of those pitfalls by exiting the ladder altogether and starting my own company where I take a fine-toothed comb to the process of onboarding new hires. Time will tell if the demands of scaling a company end up sidelining my desire for good hires, but software is still a somewhat nascent field, so the sky's the limit if you ask me!
> the human brain is weak, and it's hard enough to not get caught in the wrong local maxima without facing up to the immense challenge of choosing between global maxima in the incomprehensible problem space where software meets human needs in the hybrid meat/mental/cyberspace ecosystem where they exist and evolve over time.
This is a good way of putting it and it's why it's the problem I've chosen to work on for my first product as a startup founder. I call it Pidgin, and my vision for it is a GUI tool for refactoring and even writing code in a more deterministic and reliable way. I spend my mornings before my day job working on it. I think the fact that I think about code quality intensely at all hours of the day probably contributed to the emotional tone of my initial comment. :)
> It's simple stuff like putting together a base Modal display component for re-use instead of copy/pasting that div with the inline-styled zIndex of 99,999 for the umpteenth time.
Is this on the critical path to solving more ambitious goals or just the Sisyphean task you've appointed yourself? It's fine to take pleasure in the small details, and if you want to set up a company to feed that obsession more power to you. However I will go out on a limb and predict two things: 1) unless you are independently wealthy, you are going to stop worrying about this level of detail pretty quickly once you are focused on how to make enough money to afford to hire talented engineers and 2) if you micro-manage style and details to this degree, you may find it hard to retain the level of talent you aspire to.
I took a look at your github profile and looked through your csv_builder repo project in particular. It's good, clean code. Well done! Given that you apparently understand what clean code looks like, I can't help but feel I may be misrepresenting the nature of the 'bad code' I provided an example for. Let me expand on my modal example and perhaps you'll better understand things.
Here's a snippet from the code I had in mind when I wrote my comment:
This was copy/pasted from some other modal in the application. Have you ever heard of the DRY principle? Don't Repeat Yourself. Now, I could wax poetic about just how far one ought to go to embody this principle when considering the time pressures and business demands in a startup environment, but I don't think it should be controversial to say that repeating those 11 lines throughout the codebase for every modal is a Bad Idea, especially considering they could have used the much simpler one-liner, <Modal>.
I won't go deeper into that because that seems like a pretty air tight assumption, but feel free to let me know if it's an invalid assumption on your part nonetheless.
Now, one might say, maybe he didn't know about the other component? And, in fact, I think this was probably the case. And so I can, to some extent, forgive him for copy and pasting due to ignorance of the other component. It's a big codebase and it's impossible to stay up to date on all changes all the time. Nevertheless, this isn't an isolated case, this kind of sloppiness is the rule in the codebase rather than the exception.
> 2) if you micro-manage style and details to this degree, you may find it hard to retain the level of talent you aspire to.
Good engineers make these kinds of mistakes occasionally, it isn't their default mode of programming. I wouldn't have to worry about micro-management because the need for it, at least of the variety concerning this particular example, would never arise. Surely you understand that bad programming can and does exist?
> 1) unless you are independently wealthy, you are going to stop worrying about this level of detail pretty quickly once you are focused on how to make enough money to afford to hire talented engineers
As long as I am responsible for the construction of any kind of software, I will never stop worrying about these kinds of errors because over time they are immensely costly. Team velocity slows drastically due to the unreadable, verbose and repetitive nature of this kind of code. That is unless, of course, the product I'm building manages to embody the vision I have in mind for it and appropriately and sufficiently addresses these kinds of issues such that they are impossible to make anymore. If and when that happens, I will stop worrying because I will rest easy that an automatic process catches these errors and I will be ready to dedicate my brain power to other more interesting problems.
I have no way of knowing exactly how much, but my suspicion is that, in aggregate, sloppy coding such as this likely costs many, many companies building software around the globe millions or even billions of dollars worth of development time every year. I want to alleviate that waste with Pidgin.
And, yeah, devtools aren't exactly the cash cows of software, but they do exist and are occasionally successful. I've got a whole list of SaaS companies in the space, but for brevity I'll simply drop a reference to one I found on HN just this evening. It's called Replay (https://www.replay.io/), and it's a standalone time traveling debugger for web code. They're backed by...
I think this is par for the course in that subfield.
Part of it is the speed at which things change, both in the software ecosystems and in the change requests to sites. The reliance on comprehensive frameworks (which often need tons of workarounds and glue anyway) also reduces people's desire or experience in actually constructing good abstractions.
Compound this with the fact that startup environments generally don't care about anything other than shipping & pivoting, and want to defer all aspects of longevity.
The same problems exist all over the place, but I think are pretty prominent in these situations in particular.
How do you typically screen for such personality characteristics? I'm curious since my current role involves interviewing a lot of engineers. I too have noticed similar traits in others in the past and I shamefully admit that I myself was pedantic about silly semantics and syntax in code reviews. Sometimes criticism of someone's code was warranted, and sometimes it wasn't and it was just me being picky (I like clean, readable code).
Being picky is not the problem. Being picky and detail oriented is good quality to have as a developer.
We aren't artists, engineering is exacting field, more details you notice and remember better you can be.
The character trait you should look for how do they present that feedback, is it constructive , is it positive , the kind of language they use and how patient they are able to be.
One interview technique/tool that works for me is asking them to review some sample code mine or a junior devs.
It is easy to gauge how many kinds of issues, how many are actually important and how well they present feedback.
The other technique that worked for me is when I review their code in a interview, people who are agressively bad almost take it poorly and can't take feedback positively .
Interviewing is not 100% representative of on job behaviour, however this helps a lot in filtering out
"These people don’t last long and I have gotten pretty good at screening out this personality in interviews after having it cause huge issues at two different places I worked."
Well, now everyone is curious for more details on this. Do share :)
>Stop being an asshole, and find non-assholes to work with.
I think the process of realizing he needs to do that and doing it is what he's describing in his article. Generally people don't open up about how their behavior is toxic unless they recognize it as a problem. And, though my experience with Russian culture is limited, I understand that competitiveness and criticism is the rule, at least in the STEM fields there. So this guy has been behaving the same way (most) everyone else does, but now he's realizing it's bad and trying to fix himself. I don't think he deserves the criticism he's getting in the comments here. We all start somewhere, and trying to improve should always be encouraged.
We are all products of our environment/the past. Things change when someone says "I don't like what I've become and I don't want to be this way anymore."
Those folks often get undue criticism, especially if they choose to speak up in hopes of fostering positive change in others.
Same, I've never felt anything like this. I find code review tedious and the only thing I really care about is if their code is properly tested or not.
> I want to learn things, but code reviewing bad code doesn't teach me anything - I want to work with people who can teach me things when they review my code just as often as the reverse.
I mean maybe you're working on extremely repetitive code, but the idea that you don't learn anything from reviewing bad code, and hopefully writing comments designed to help not humiliate, seems pretty bad to me also...
You want to learn from others who review your code, but become angry when you have to teach those whose code you review? That seems bad...
It's certainly bad to get angry - I mentioned that there. I should clarify the anger happens when it's repeated more than on a one-off. E.g. if you have the same sloppy signs for 6 months (where I can tell by reading the code that you didn't even try to run your code locally), then I'm not learning anything and I'm getting frustrated because you're clearly not taking feedback to heart.
I try to avoid letting this show, but it's frustrating.
Yeah, I was afraid I'd see myself in this post, because I'm known for giving tough reviews. But I don't. I feel terrible whenever I give a tough review. I agonize about how to say the difficult things that need to be said but do it in a way that won't sting as much, and I know that often, it's going to sting anyway. I feel like I failed for not helping the other developer know how to do things better in the first place. I'm still figuring out how to make my process more constructive, but I've got a long way to go.
One thing that I find that helps is, instead of saying that this thing is wrong, I just put in what would be right, and I do so with little commentary; usually `code block` -> `code block`.
Another thing that I think helps, at the CR stage, is asking after their thoughts for putting the thing together the way that they did. This DOES run into the potential problem of showing that they didn't put thought into it (or that part of it), but....
...there's a certain extent to which it's not your fault if they get to the CR and get a tough review. It's your organization's, "a month of work saves an hour of planning"-style. The developer who wrote the code getting the tough review wasn't set up to succeed, and IMHO that's more a comment on your org than any of the developers involved.
To bring it back around to your personal process, my (unsolicited, haha) advice is that if you're finding yourself giving a tough review, switch to a 1:1 review / pair programming session. Async text communication is hard and prone to miscommunication.
Sometimes I’ll look over PRs early on, notice the entire approach is bad, and leave a comment.
People often got annoyed, they would say their PR isn’t ready yet.
That’s fair, I kind of understand, my intention was to just save them time going down an incorrect path though, because I know it’s going to be difficult to convince them later they need to rework days of work.
If team work isn't in the culture at your place it's very understandable they would react like that. If they don't see the benefits at your place and they've also only had bad experience with that at their previous places it's the natural defense mechanism.
You could do the first step for example. Put up your own stuff for early on RFC. Invite them to work collaboratively on it with you and give early feedback and see how you react. Basically: be the role model and let them follow.
Not guaranteed to work but it's one way to start some of it.
Sidenote: most teams don't actually do team work. It's a bunch of people thrown together that work on more or less loosely related stuff but everyone is out for themselves. It's a matter of the environment they have to work in in most cases. Individual metrics driven productivity culture. You can't expect someone to do good team work and collaboratively come up with the best approach when their incentives from management's side is to get as many PRs merged as they can to get that bonus and a good end year rating etc.
> I'm constantly looking for ways to do less tedious work, through automation and such, and code review for bad code is the worst sort of tedious work.
What kind of automation or tools are you using for code review?
I concur. I'm not a coder but I do review things every 2 months as part of the release cycle, and whenever I see mistakes, especially silly mistakes, I don't feel smart at all, it doesn't even cross my mind.
I feel, in order: disappointment - because how could they make this stupid mistake, then anger - why the f do I have to waste time to correct stupid mistakes, then a bit hateful - we need to fire this person and hire someone competent, then sometimes a bit scared - what else has this person got wrong and I missed, then a mix of all of the above.
What would make me feel good, as far as reviews go, would be to say "hey, this person did this way better than I would have".
If you want some empathy with the guy whose code you are reviewing, go pull up some of your own code from a few months or a year ago. Do a mock code review on it.
Maybe everyone else only writes good code, but this works for me every time. Even after over 30 years coding, I still have no problem seeing how imperfect the code is that I wrote as recently as last year.
And don't ever make it personal, anyway. Adopt a neutral tone, phrase comments abstractly as how they relate to team goals, how the code might be made clearer, etc. Don't accuse, don't belittle, don't try to look smarter than you are. And try to recognize the difference between the stuff that actually matters, the things which will actually be a headache down the road, and the pointless details that are just not how you'd have chosen to code something.
Having an auto-formatter like Prettier or `go fmt` has been a huge boon and productivity gain for the teams that I've worked with. No longer having code nitpicks on formatting has saved so much time going through reviews, and it really increases the signal to noise ratio on pull request comments too.
Would definitely recommend those for any teams that have a similar teammate as the article author on their team.
Yeah, we mostly do Python and I've really been trying to push the use of black. I don't actually like some of black's formatting choices (OK, it's grown on me) but it's better to have a standard and stick to it.
Ouch. I’ve had a few cases of developers on the verge of being fired because of bad code causing friction with the team.
Code reviews never help in those cases. There is always an underlying cause: the developer is trying to rush to prove something, or maybe over-engineering for the same reason. Maybe he has some issues at home, or isn’t sleeping well. Maybe it’s just lack of experience. Maybe it is a personality issue and the developer is trying to save face by sticking to his guns.
Either way, I find that it is much better to have private discussions with the developer to increase the code quality via mentoring, or pairing. It helps removing the defensiveness, doesn’t make them feel publicly humiliated and doesn’t spend the time of the team with the review-fix cycle.
I'm not a super-coder; give me a JS project of moderate complexity and I'd have no idea what's going on or what I'm supposed to do with it. But as a data scientist I do have the reputation of being good at coding even among some of the engineers at my company.
And some of the code produced by both data scientists and engineers ... it's, hm, not great? Just between you and me? So I'm glad I've never had the inclination to tear anyone to shreds. Indeed, part of my job used to be to help people with their coding problems, and I loved doing that.
But sometimes when I'm reviewing code ... it's very hard to bite my lip and not call out every little issue, even though it would just be too much and most of the "problems" aren't actually very important. I try to impress upon people that developing good code is useful when I lead training sessions, but it's hard, especially for people who'd rather be doing data science than coding.
Funny. I'd say that if somebody can't work with or maintain code written by others then that's the sign of a weak programmer. If all code you work with has to be written exactly as if you wrote it yourself then that's a weakness.
90% of a code review is not about finding THE BUG in the PR but asking questions and making the author find his own bugs or learning something from him.
If you are performing a code review hunting for bugs you already lost your humility and think you are better then the PR author.
Have you ever written some text, and re-read it, and it sounded just right, and had someone else read it, and they said "what does this mean"? Sometimes the words you write are sufficient to trigger in your own head the thoughts you've already had, but not enough to make someone else think those same thoughts.
Code can be like that, too. You write code, and it's enough to convince you that it does what you think it does. But someone else reading it can sometimes see the holes that you don't see.
Assuming that person X writes bug-free code is bad, whether X is "you", "me", or "that co-worker".
I don't have to be better than the PR author. I just have to be decently good, and be a different person. That's enough.
And of course I'm looking for bugs. That's the absolute first thing to look for!
Now, true, if I can help the author be able to see their own bugs (asking questions, maybe), then that's probably better than me just lecturing them. Teach them how to think so that they can see it themselves next time.
I'm not saying that you will never find bugs in someone PRs I'm just saying it shouldn't be your first intention, your first intention should be to understand it and be convinced by it. Finding bugs will be a side effect of that.
If you reading the code to execute it in your head and find all the weird combination that could trigger a core dump, you should probably check if the author wrote tests for those use cases instead and comment about missing tests.
Genuine interest in others work can go a long way compared to imagining how would you solve it like the article author said.
In my estimation, most places have the opposite problem: everybody approves the code without looking through it, and the only comments that are left are stylistic problems.
Good code reviews spot real bugs and provide solutions and explanations that help the owner to improve their branch. It's completely constructive and feels like going out of your way to way to help them and paying attention to what is meaningful -- the vibe should be a bit like "doorman at a posh hotel providing excellent service", so not disrespectful at all. Also, if you're nitpicking, it's a sign of lack of focus...
I had an experience in my first year of grad school that has informed my code reviews and my general attitude toward reviewing others' work ever since.
I was taking a graduate-level introduction to mathematical logic. In the class we learned about a bunch of different propositional axiomatic proof systems, and then we proved properties like soundness and completeness for them. The class was organized around class participation: everyone was required to do at least one proof at the board in front of the class and every session consisted almost entirely of students doing proofs at the board.
I'd been out of school, working, for eight years before grad school and I found this all very intense and unfamiliar. I did my required work at the board, choosing an easy problem, but otherwise mostly sat in the back and watched. There was, however, a cohort of very intense aspiring logicians who sat in the front row and took deep interest in every proof.
One day, a student went up with a particularly challenging problem. From the start it was clear that he was taking an unorthodox approach to the proof. The front row went nuts, interjecting and kibitzing on practically every line, telling the guy different things that he should do instead of whatever it was that he was doing. The guy was a good sport and tried to justify everything and answer all the criticism, but it seemed like he might not get through the proof before the class ended.
The prof who had been watching quietly from the side of the class, let it go on for a while, then stepped out and quieted everyone.
"I need you to remember," he said to the front row, "that we are the audience. It is not our job to prove this theorem; it is this gentleman's job. Our job as the audience is to listen to him, and at the end, decide if we are convinced. He may do things differently that you would have done, but that doesn't matter, as long as in the end we are convinced."
That was twenty-three years ago, but it stuck with me. In programming, there are often many different ways that a problem could be solved, but there is almost never One True Way™, even though there are often many voices that would like to convince you that their way is the right one. My job as a reviewer of code, or really anything, is not to try to get the other person to do what I would have done, but to decide if I'm convinced that the way that they've chosen is good enough, and, if not, to ask for changes that would convince me.
Sure, sometimes a PR is so bad that it needs to be completely rewritten, but in my experience that's rare, and in that case the feedback should probably be given in person and in private or in a friendly, sympathetic group.
This except there is no right way and there are 'n' equally good ways of solving any real world problem. It's only worth pointing out flaws if something is genuinely bad and I mean bad. Otherwise you are using a gut opinion to trash someone else's emotional health.
Yay! you found an edge case that breaks a piece of code that will never get hit while the other million and 1 edge cases that haven't been found are still out there. Yet you drag it out from the dark and show it off for all to see like some damn trophy. Yay! look I am great I found one more thing wrong than the other guy. No matter that we are all swimming in a sea of wrong and every one is completely clueless. Only slightly less clueless than those who don't even realise they don't have a clue.
It follows from a simple argument - every programmer and every company has a different view of 'good'. I've seen it, I've worked in many. Just as every religion cannot be right, every weird view on correct programming style cannot be correct.
I always look at code reviews or just reading / talking about each other's code as a way for us to just get on the same page about how we do things. Make everyone's life easier. "Hey man when we're looping through that thing here is how we usually do it." "Ok cool, I'll do that." Or maybe we have a discussion on why I did it differently this time and how we might address that.
Much of the time I don't "care" or feel strongly what the change or answer is. If we're all doing the same thing / spot a bug sooner, it's just easier for everyone.
But it seems code reviews to some are this sort of battle / proving ground / have a lot of elements of status. I don't get that...
198 comments
[ 0.24 ms ] story [ 267 ms ] threadThe real danger of confusing the two is that real skill dulls if you let overconfidence take over, since the very thought of being "better than others" prevents you from accepting that maybe there are things that you don't know or haven't considered.
For example, is "destroying" a coworker's commit in a code review really really helping? Did the person perhaps forget to consider that hiring to refill the position of a fired developer actually costs physical money (as opposed to time spent on coaching?) or that perhaps a lot of time is being spent on nitpicks without bottom-line value or that tip-toeing around egomaniacs can stunt autonomy and therefore creativity? This is not merely a matter of whether you're lulling yourself into some false sense of superiority/duty/whatever; thinking in terms of bigger and bigger pictures is an actual skill that one needs to develop when going up the career ladder.
Why should be bad PRs ever be allowed for any reason?
hell, that's a GREAT opportunity to get this "bad developer" under your umbrella and start mentoring him to be a better developer.
Teaching people cool new ways to code should be fun, not an exercise in showing everyone the size of your ego.
Define bad PR. Are you sure that your definition of "bad" is accurate?
What really matters is whether the code is solid and whether it will actually cause problems. Just because a PR isn't character-for-character how I'd write it doesn't mean it's bad. Based on the author's experience with the fired employee's replacement being just as bad, my guess is he was over defining goodness and raising the bar too high.
Bad PR to me means: bad runtime complexity, bad formatting, bad security, bad memory use, bad understandability. If it meets all those bars, which for a crud app is usually easy, then it should probably pass. I'll sometimes leave a "I would have done _x_ differently" comment on matters of taste _if I think it will help the other developer_.
This, too many people think their way is the only way. It only takes one cancelled project or one company going bankrupt (and 100,000s loc going "poof") to understand this work we do, really doesn't matter all that much in the end.
And I've learned all the small nit-pick maintainability issues made no difference. Whether or not there was a style guidelines, and whether or not it was enforced made little difference.
Things that really made a big difference were giant bone-headed architectural decisions like "just save persist this data as serialized text", "lets write all the business logic in stored procedures", "building the software on a terrible platform".
And having some type of identifiable domain model helped a little too.
It's first solved by identifying what the cause is of their poor performance. Are they in over their head? Are they having personal problems which are distracting them? Are they experiencing burnout?
I've heard people say that they enjoy code reviews because it gives them latitude to be ruthless. After all, it's for the benefit of the business to not allow suboptimal code through, and it's just code. But frankly, I think that's a lame excuse and the easy way out.
There are people behind code, and while the code might suck, the people don't. Obviously, that doesn't mean you should let it through, but it does mean you should be at least professional, and preferably compassionate in how you review it.
Ranging from "I wouldn't have implemented it that way" to "this opens up a huge security hole".
Security issues shouldn't be allowed in. But a lot of "maintainability" issues have negligible if any gains, and real costs like
it takes time to fix
could introduce new bugs
creates bad feelings between coworkers
I think that this is called projection.
https://medium.com/@sandya.sankarram/unlearning-toxic-behavi...
I've found that I usually start with the "correct" way and end up regressing to the "wrong" way as more and more work piles up, stress builds and deadlines approach. I guess the "correct" way only works if the organization actually prioritizes it and gives reviewers the time to do it the "correct" way no matter what stage the project is in.
Some might say, oh it's hazing, well fuck you because I'm quitting. I'm not putting up with any amount of hazing at a job. If I have to spend 8 hours everyday somewhere I'm for sure not going to spend it with some asshole.
I’m craving some constructive feedback.
I haven’t asked them. I should ask for more feedback.
I haven’t asked because:
I was a latecomer to the team and they had an established process
Cultural differences between myself and them
Honestly, I’m not sure all of them are experienced enough to give a useful critique
I used to do that when I started working on open-source as well, doing code-reviews to learn the code base (often I would not submit the review to not create noise)
I wouldn't let this be a stumbling block to asking for more review. I think it's like writing an article or paper - once you've invested a lot of time into the thing it gets harder to see the warts. Having another set of eyes can really help catch that stuff, even if they aren't at the same level of expertise as you. Back in my research days the department I was in had a person who's job was basically reading research papers and providing feedback - in this case she usually focused on the paper structure, clarity of phrasing, and of course grammar - she was an expert in writing not in CS. Yet looking back a drafts of papers before and after she had given input shows a massive improvement, even if the technical content was unchanged. Similarly a smart intern can be a great source of input for making my code clearer and better, even if the algorithm doesn't fundamentally change.
Also, if like me you are prone to falling into ego traps, inexperienced folks can bypass the ego's defenses. A simple "hey why did you do it $complicated_way instead of $simple_way?" can hit a lot harder than "hey make this $simple_way" from a mentor. I think it's because I've invested time into teaching the intern and when they ask a good question like that my ego gets a boost from "I taught that kid well" to balance out the "OMG YOU THINK I'M TERRIBLE AT THIS LETS FIGHT" response. (It may also be tempered because I tend to view the intern's questions as genuine requests for input, so I start thinking of the explanation and realize $simple_way will in fact work just fine.)
So: is your code naked, on fire and carrying a bomb?
Likewise, if there's no comments on a pull request, it hasn't been read...
I mean, sure it doesn't render well as HTML, but its readable with the code and the rest of the project has none so...
I've been on the receiving end of enough toxic code reviews to develop an aversion to do them. So when asked to do them, I really just ask, did you test it?, do you want to demo it to me?, then approve it.
When people demo to me, I can see their pain points, because they always point them out. "Oh yeah, I was having trouble getting this to work, as you can see, it's still a little slow" and it gives them an opportunity to ask for advice or merely to vent.
So if you want good, honest feedback, a demo is the way to go. Honestly, it's hard to really grok what a bit of code is doing just by reading it. And who has time to pull code and run through it solo when doing a review?
Instead of "please write this code, submit a pull request, and I'll tell you what you did wrong," I have recently been saying to new coders "take a shot at this problem, and when you've got something working let's get together and refine it, there is some context it will take time to learn so we'll probably need to make a lot of changes or even rewrite it before release."
That way they know what's coming (a lot of changes), they know why it isn't a sign they're inadequate, and it's more about working together than passing judgment.
It has also helped reduce the massive delays while junior developers agonize over making something perfect.
It worked out fine I think and, mind you, it used to be a lot harder to push out a fix. A lot harder.
The code review was preloaded.
It’s better when code reviews are suggestions, the reviewer doesn’t nitpick (or at least fixes that stuff themselves), and small things like formatting are enforced by a linter.
(It might also mean your team doesn't care about anything any more, in which case look for a better opportunity, unless you want to just cruise along for a while for whatever reason, in which case turn into a bare minimum performer like the rest of your team).
I'm a senior engineer in a known company and my target is always to leave no comments on a patch. I only give question-comments if I absolutely must know something before merging, or please-fix comments if something is definitely wrong and I can prove it. Also, if it's a part of the system that poses little risk (for example, if it breaks I can assign the bug back to the author and patiently wait for a fix with no serious damage) my tolerance for bugs greatly increases. The last part I'm not too proud of but frankly I don't have time to review everything. A "fix" for the last part would be to find a lower rank reviewer-buddy who has more time to share knowledge.
One of my peeves is a review that points out a bug/incorrectness without offering proof, or suggesting the fix/correct way. The reviewer might observe a code smell, but it's so much better dealt with a question-comment, e.g. "did you try?" vs "this looks wrong".
I generally aim to make every comment a question ("can we..."), and not make assumptions that the author didn't think of something already. Unless I just stating an objective fact that is clearly unknown e.g. "this duplicates library function x...".
But that's not exactly what I meant with question-comments, I sometimes genuinely don't know why are they doing something like this, or have to ask for additional testing/evaluation/analysis/profiling results before I can decide that it's most likely not going to blow anything up. And as long as there's low probability of damage (or rather low expected value of damage) I am happy with the change :)
What really helps is a style guide. It shuts most of those pointless style nitpicks, God I hate them.
* in my experience, of course.
This person is so confident in himself, that he thinks his code review takedowns are so brutal, yet effective, that he is ruining lives. It almost comes off as "I'm so handsome that I ruin people's lives with my looks and I'm sorry."
And then there is this...
"I was mad that, while I spent my nights learning F#, my daughter started calling everyone around “fathers”. And this guy, instead of getting better at his job, went home to his children. And I wanted to punish him."
Uh, whose life is being ruined?
The recounting of "bullying"/hazing on forums definitely resonates as well, and he's just remarking that he'd become what he'd hated.
This is more of a reflective piece rather than a glorification of his behavior.
>> My personality today isn’t my disease. It’s a disease of the whole industry... just stop being that. It’s quite easy, actually.
I've had the same thing the other way, where I feel that I usually give kind and fair code reviews (no nitpicking, etc) but have had former colleagues that get genuinely upset and push back really hard when I make any sort of comment on their PR.
Because it creates technical debt, that's why people get frustrated with shit code. They just aren't properly equipped how to convey/teach/train/fix it without coming across like an asshole.
Does it ? Sometimes, the code reviewer has taken less time to analyze the problem and has not seen all problems a simpler solutions would bring.
In my opinion the first priority of code review should be to prevent mistakes in the code at hand
I disagree. The first priority of code review is WHAT problem is it trying to solve and does it actually solve the problem.
If it's not even solving the problem correctly, pedantic nitpicks are inconsequential.
My mental model is that some people feel like they must comment on a code review to show that they've done something. They don't actually care, but just want to write a comment or two and you can shrug those off.
If following this approach you do need to be careful to only ignore genuinely useless comments though. Often a good way to tell is just to chat with the reviewer. That has the added benefit of the reviewer learning that useless comments will create more for them because you'll be by to chat about their useless comments.
In other words, I think it's correct to not consider every comment to require a change unless it really needs it, but that doesn't mean a comment that doesn't require a change for the current work should necessarily be ignored because of that.
- is a developer making the same set of mistakes? send them information on a better strategy to try overall
- how bad is this mistake? Is it just something that I know how to do better, or legitimately a broken way of thinking?
- what is worse, a slightly inefficient or unpretty algorithm, OR someone researching the problem / solution
Point is, 90% of my comments were meaningless. I really comment when things look wrong -- Did they misunderstand the intent of the ticket, did they verify everything with product because this looks weird, did they check the edge cases, did they forget about this other behavior, etc. All that I just mentioned is the real meat-and-potatoes. Code-fu is important, but less important than the other stuff. And if you are a calm, trusted, and mentoring team member, those same people will come to you to up their code-fu voluntarily.
I do think thorough feedback is important, that's how I've learned the most after all. But typically now if I I'm going to write more than 5 comments, I meet with the other developer in person instead. That way the code problems are solved faster and it avoids any sort of public humiliation.
I really can't relate at all. I feel the opposite. I often feel angry (I realize this is not a GOOD response either!) and always feel sad. I don't want to have to do more work. I don't want a reminder that my previous feedback didn't click with them.
I'm constantly looking for ways to do less tedious work, through automation and such, and code review for bad code is the worst sort of tedious work. I want to learn things, but code reviewing bad code doesn't teach me anything - I want to work with people who can teach me things when they review my code just as often as the reverse.
My takeaway from that has been to try to intercept people earlier in the planning and designing and start-of-coding process. If a bunch of bad code gets to code review, someone failed at explaining what this thing needed to do and how it should be done beforehand. The article claims this is always just itself a confrontational form of argument, but... it doesn't have to be. Stop being an asshole, and find non-assholes to work with.
That emotional reaction in combination with conviction everyone has the same one disqualify him.
The truth is that people's emotions can be a lot more different from yours than people may be willing to imagine.
No one is perfect. But not everyone derives 'insane pleasure' from criticizing, either. And if you do, that may not be obvious.
I agree with the 'stop being an asshole' line, but to do that we need to help people realize when their qualia is way out of the ordinary. If you think everyone else is power-tripping, too, then will you think you're really being an asshole, or are you just doing what everyone else does?
It's scary to think that you might have something else you think everyone else does, but is really just you accidentally being oblivious. Speaking for myself, I'm going to try to be more mindful of this trap in the future.
This reads like American Psycho. I can't believe this guy is for real. You have nothing but your job, so - at your job - you try to make everyone you work with miserable? What? Why?
Have you considered trying to make your life enjoyable ever?
When one company buys another the quality of the code is a calculation in the sales price. But it's a small one, and it takes a real dumpster fire to move that needle.
These people don’t last long and I have gotten pretty good at screening out this personality in interviews after having it cause huge issues at two different places I worked.
As a rule, so far throughout my 5 year career as a full stack web developer, my coworkers have neither understood or cared about what clean code entails.
I don't berate, I don't criticize, I've taken to mentioning it once and then doing my utmost to never mention it again in an attempt to avoid labeling myself as the odd one out.
Granted, I've turned down a handful of much higher paying (and presumably higher quality teams) for various reasons. At first, it was because I was a bootcamper and high paying jobs are difficult to pick up out of the gate without the appropriate piece of paper. Since then, I've worked for various organizations on a consulting basis, and most recently landed at an early stage YC startup. Pay isn't great, but I have aspirations to start my own startup, so I'm cool with the pay cut for now in return for the opportunity to learn the ropes, business wise. Founder knows what he's doing at least.
But the devs, man. They just don't care. 500-1500 line React function components are the norm. No standardized way for retrieving data from the back-end. I dread assignments that require me to modify or otherwise integrate with previously written code, because the best way I can come up with to figure out what the fuck is going on is to rewrite it sanely. I'm being a little hyperbolic, I do my best to avoid rewriting wherever possible, but I'm not mis-characterizing things by much.
It's honestly depressing, especially considering I've had multiple other teams offer triple what I'm making right now, but neither of them were pre-seed startup SAAS companies which this one is, so here I stay for the time being.
Invariably, I garner a reputation as "that guy" on any team I work on, although granted this is only the second multi-dev team I've ever worked on. Still though, it feels terrible to get a bad reputation for writing good code. Founder likes my work ethic and output though, so I guess I've got that going for me.
Apologies if this turned into a bit of a rant, it just started pouring out, ha!
It does seem to be something the big co. interview gauntlet is designed to weed out, whether it's worth it or not. I'm sure it's still relatively common, but probably a lot rarer than at the median startup.
You sound still early career enough that the years will likely mellow you out and teach you to better appreciate the ups and downs of building things fast.
You know what’s worse than no abstractions? The wrong abstractions.
I dread cleaning up solutions that I designed earlier where with hindsight, I made a lot of wrong assumptions. I still make wrong assumptions today, they are just designed less brittle.
However one thing I have learnt is more coupled the code is with no or poor incomplete abstractions less likely anyone is going to change it.
In the interest of shipping fast I would rather have some abstraction today rather than none, simply because nobody fixes spaghetti code which is already working.
Building new things start slowing down more and mode and you end up taking weeks for what should have been an afternoon's effort .
There are no right answers, however sometimes wrong abstractions can be marginally better than none.
You know what's worse than the wrong abstractions? Copy and pasting your inline-styled div with a zIndex of 99,999 for the fifth modal in a row because you can't be assed to write a damned display component in anticipation of the next time you'll need to put a Modal together.
Levels to it all. Keep track of painful modules and have strong intent to fix them. Prioritization is a mf.
> But the devs, man. They just don't care.
Well, maybe. Or they just don't know how to write better code.
Either way, the bigger problem is the team management that allows this.
> I do my best to avoid rewriting wherever possible
Why not? If the other devs truly don't care, you could take it upon yourself to clean up the code base little by little when you can.
I used to do this simply because it makes the code difficult to read for me to review if for nothing else.
Linters and style check tools only go so far, and bottom line is you have to ship software not write complex and expensive to maintain tooling, so automation beyond a point is not a solution.
The devs ended up relying on me as the their clean up guy for all the code all the time. I also becoming gatekeeper/bottleneck of sorts.
It is used make blame and dissection more difficult as more times than not I had cleaned up the code and the last one or two commits are mine.
Also the time spent in reviews reduce and you tend to miss important things as you have focused on what should be basic hygiene so much.
One of the reasons I contribute a lot less to production code is this. It is a losing battle if most devs don't care about it as much as you .
Even now the occasional pull request I would make would be peppered with such corrections , it is just automatic reflex for me, but it doesn't make that much difference.
Don't do this, people either have to value the cleanliness enough to do it as well, or you have to tolerate what is the common minimum or find another team/organization.
If the manager is on your side, it's possible but not fun.
Another possible reason could be that they know it’s temporary. A lot of startup code gets rewritten at least once in the first 5 years, there is some art to knowing what code might stick around and be worth the effort to do well, and what you should just bang out fast and move on.
It sounds like it’s possible you haven’t been able to have a conversation with your colleagues about their choices, which might be worth having candidly if you can find the opportunity to do so. If it is ignorance or negligence better to know and decide how you want to proceed if that makes sense.
Once you become competent enough to identify clean code, understand that your perception of clean code is colored by your own experience and current mental context. As you learn, you will undoubtedly be aghast at your own code that you wrote at any time before 6 months ago. After you've been at it for a couple decades you will come to the horrifying conclusion that your mental model is always incomplete, the human brain is weak, and it's hard enough to not get caught in the wrong local maxima without facing up to the immense challenge of choosing between global maxima in the incomprehensible problem space where software meets human needs in the hybrid meat/mental/cyberspace ecosystem where they exist and evolve over time.
The other implication is that no two engineers will ever see things the same way. It's not purely about technical strength—a weaker engineer might come up with a better solution given more time to think about it, or due to some other blind spot or skillset gap from the more senior engineer. Also, there are many issues of style and perspective where the important thing is consistency more so than some specific choice. In those cases there's a high risk of bike-shedding, and it leads to a fundamental truth of large scale software development which is that in practice you will never get everyone on the same page. Of course there are controls for this, but unless you have the budget and operational scope constraints of NASA (or perhaps medical device manufacturers) you can't actually enforce this without losing so much development velocity that your company is utterly uncompetitive in the market.
As you move up the seniority and scale ladder to larger projects and architectures, you will realize that you still only have 24 hours in a day, and it doesn't matter how smart or how right you are, the only tool in your built to increase your impact is a simple cliche: "pick your battles". There is absolutely no upside in nitpicking every line of code that could have a conceivable improvement. If you want respect from your colleagues, you need to get good at filtering what is important and what isn't, and only pointing out the most important issues, and doing it as early in the process as possible.
I appreciate the reminder and the helpful tone of your comment in general!
> As you learn, you will undoubtedly be aghast at your own code that you wrote at any time before 6 months ago.
Just checked, and my code from 6 months ago looks identical to what I write at work. My programming style went through a few different phases but its variability has mostly leveled off since I've started writing in a mostly-functional style.
> It's not purely about technical strength
It's simple stuff like putting together a base Modal display component for re-use instead of copy/pasting that div with the inline-styled zIndex of 99,999 for the umpteenth time.
> As you move up the seniority and scale ladder to larger projects and architectures
I hope to side-step a lot of those pitfalls by exiting the ladder altogether and starting my own company where I take a fine-toothed comb to the process of onboarding new hires. Time will tell if the demands of scaling a company end up sidelining my desire for good hires, but software is still a somewhat nascent field, so the sky's the limit if you ask me!
> the human brain is weak, and it's hard enough to not get caught in the wrong local maxima without facing up to the immense challenge of choosing between global maxima in the incomprehensible problem space where software meets human needs in the hybrid meat/mental/cyberspace ecosystem where they exist and evolve over time.
This is a good way of putting it and it's why it's the problem I've chosen to work on for my first product as a startup founder. I call it Pidgin, and my vision for it is a GUI tool for refactoring and even writing code in a more deterministic and reliable way. I spend my mornings before my day job working on it. I think the fact that I think about code quality intensely at all hours of the day probably contributed to the emotional tone of my initial comment. :)
Is this on the critical path to solving more ambitious goals or just the Sisyphean task you've appointed yourself? It's fine to take pleasure in the small details, and if you want to set up a company to feed that obsession more power to you. However I will go out on a limb and predict two things: 1) unless you are independently wealthy, you are going to stop worrying about this level of detail pretty quickly once you are focused on how to make enough money to afford to hire talented engineers and 2) if you micro-manage style and details to this degree, you may find it hard to retain the level of talent you aspire to.
I took a look at your github profile and looked through your csv_builder repo project in particular. It's good, clean code. Well done! Given that you apparently understand what clean code looks like, I can't help but feel I may be misrepresenting the nature of the 'bad code' I provided an example for. Let me expand on my modal example and perhaps you'll better understand things.
Here's a snippet from the code I had in mind when I wrote my comment:
This was copy/pasted from some other modal in the application. Have you ever heard of the DRY principle? Don't Repeat Yourself. Now, I could wax poetic about just how far one ought to go to embody this principle when considering the time pressures and business demands in a startup environment, but I don't think it should be controversial to say that repeating those 11 lines throughout the codebase for every modal is a Bad Idea, especially considering they could have used the much simpler one-liner, <Modal>.I won't go deeper into that because that seems like a pretty air tight assumption, but feel free to let me know if it's an invalid assumption on your part nonetheless.
Now, one might say, maybe he didn't know about the other component? And, in fact, I think this was probably the case. And so I can, to some extent, forgive him for copy and pasting due to ignorance of the other component. It's a big codebase and it's impossible to stay up to date on all changes all the time. Nevertheless, this isn't an isolated case, this kind of sloppiness is the rule in the codebase rather than the exception.
> 2) if you micro-manage style and details to this degree, you may find it hard to retain the level of talent you aspire to.
Good engineers make these kinds of mistakes occasionally, it isn't their default mode of programming. I wouldn't have to worry about micro-management because the need for it, at least of the variety concerning this particular example, would never arise. Surely you understand that bad programming can and does exist?
> 1) unless you are independently wealthy, you are going to stop worrying about this level of detail pretty quickly once you are focused on how to make enough money to afford to hire talented engineers
As long as I am responsible for the construction of any kind of software, I will never stop worrying about these kinds of errors because over time they are immensely costly. Team velocity slows drastically due to the unreadable, verbose and repetitive nature of this kind of code. That is unless, of course, the product I'm building manages to embody the vision I have in mind for it and appropriately and sufficiently addresses these kinds of issues such that they are impossible to make anymore. If and when that happens, I will stop worrying because I will rest easy that an automatic process catches these errors and I will be ready to dedicate my brain power to other more interesting problems.
I have no way of knowing exactly how much, but my suspicion is that, in aggregate, sloppy coding such as this likely costs many, many companies building software around the globe millions or even billions of dollars worth of development time every year. I want to alleviate that waste with Pidgin.
And, yeah, devtools aren't exactly the cash cows of software, but they do exist and are occasionally successful. I've got a whole list of SaaS companies in the space, but for brevity I'll simply drop a reference to one I found on HN just this evening. It's called Replay (https://www.replay.io/), and it's a standalone time traveling debugger for web code. They're backed by...
I think this is par for the course in that subfield.
Part of it is the speed at which things change, both in the software ecosystems and in the change requests to sites. The reliance on comprehensive frameworks (which often need tons of workarounds and glue anyway) also reduces people's desire or experience in actually constructing good abstractions.
Compound this with the fact that startup environments generally don't care about anything other than shipping & pivoting, and want to defer all aspects of longevity.
The same problems exist all over the place, but I think are pretty prominent in these situations in particular.
I'll say things like, "I would have done that like this because of this reason, but as long as it works"
or "This needs to change right now because it's a security hole"
or "We should find a more efficient way to do this, because as the data fills up, the users will start to notice it"
I am definitely harder on new people, and people who ask me for help when they should have been able to figure it out on their own.
We aren't artists, engineering is exacting field, more details you notice and remember better you can be.
The character trait you should look for how do they present that feedback, is it constructive , is it positive , the kind of language they use and how patient they are able to be.
One interview technique/tool that works for me is asking them to review some sample code mine or a junior devs.
It is easy to gauge how many kinds of issues, how many are actually important and how well they present feedback.
The other technique that worked for me is when I review their code in a interview, people who are agressively bad almost take it poorly and can't take feedback positively .
Interviewing is not 100% representative of on job behaviour, however this helps a lot in filtering out
Well, now everyone is curious for more details on this. Do share :)
I think the process of realizing he needs to do that and doing it is what he's describing in his article. Generally people don't open up about how their behavior is toxic unless they recognize it as a problem. And, though my experience with Russian culture is limited, I understand that competitiveness and criticism is the rule, at least in the STEM fields there. So this guy has been behaving the same way (most) everyone else does, but now he's realizing it's bad and trying to fix himself. I don't think he deserves the criticism he's getting in the comments here. We all start somewhere, and trying to improve should always be encouraged.
Those folks often get undue criticism, especially if they choose to speak up in hopes of fostering positive change in others.
I mean maybe you're working on extremely repetitive code, but the idea that you don't learn anything from reviewing bad code, and hopefully writing comments designed to help not humiliate, seems pretty bad to me also...
You want to learn from others who review your code, but become angry when you have to teach those whose code you review? That seems bad...
I try to avoid letting this show, but it's frustrating.
Another thing that I think helps, at the CR stage, is asking after their thoughts for putting the thing together the way that they did. This DOES run into the potential problem of showing that they didn't put thought into it (or that part of it), but....
...there's a certain extent to which it's not your fault if they get to the CR and get a tough review. It's your organization's, "a month of work saves an hour of planning"-style. The developer who wrote the code getting the tough review wasn't set up to succeed, and IMHO that's more a comment on your org than any of the developers involved.
To bring it back around to your personal process, my (unsolicited, haha) advice is that if you're finding yourself giving a tough review, switch to a 1:1 review / pair programming session. Async text communication is hard and prone to miscommunication.
People often got annoyed, they would say their PR isn’t ready yet.
That’s fair, I kind of understand, my intention was to just save them time going down an incorrect path though, because I know it’s going to be difficult to convince them later they need to rework days of work.
Not sure what the solution is.
You could do the first step for example. Put up your own stuff for early on RFC. Invite them to work collaboratively on it with you and give early feedback and see how you react. Basically: be the role model and let them follow.
Not guaranteed to work but it's one way to start some of it.
Sidenote: most teams don't actually do team work. It's a bunch of people thrown together that work on more or less loosely related stuff but everyone is out for themselves. It's a matter of the environment they have to work in in most cases. Individual metrics driven productivity culture. You can't expect someone to do good team work and collaboratively come up with the best approach when their incentives from management's side is to get as many PRs merged as they can to get that bonus and a good end year rating etc.
What kind of automation or tools are you using for code review?
I feel, in order: disappointment - because how could they make this stupid mistake, then anger - why the f do I have to waste time to correct stupid mistakes, then a bit hateful - we need to fire this person and hire someone competent, then sometimes a bit scared - what else has this person got wrong and I missed, then a mix of all of the above.
What would make me feel good, as far as reviews go, would be to say "hey, this person did this way better than I would have".
Can you tell I hate doing reviews?
Maybe everyone else only writes good code, but this works for me every time. Even after over 30 years coding, I still have no problem seeing how imperfect the code is that I wrote as recently as last year.
And don't ever make it personal, anyway. Adopt a neutral tone, phrase comments abstractly as how they relate to team goals, how the code might be made clearer, etc. Don't accuse, don't belittle, don't try to look smarter than you are. And try to recognize the difference between the stuff that actually matters, the things which will actually be a headache down the road, and the pointless details that are just not how you'd have chosen to code something.
Would definitely recommend those for any teams that have a similar teammate as the article author on their team.
Code reviews never help in those cases. There is always an underlying cause: the developer is trying to rush to prove something, or maybe over-engineering for the same reason. Maybe he has some issues at home, or isn’t sleeping well. Maybe it’s just lack of experience. Maybe it is a personality issue and the developer is trying to save face by sticking to his guns.
Either way, I find that it is much better to have private discussions with the developer to increase the code quality via mentoring, or pairing. It helps removing the defensiveness, doesn’t make them feel publicly humiliated and doesn’t spend the time of the team with the review-fix cycle.
And some of the code produced by both data scientists and engineers ... it's, hm, not great? Just between you and me? So I'm glad I've never had the inclination to tear anyone to shreds. Indeed, part of my job used to be to help people with their coding problems, and I loved doing that.
But sometimes when I'm reviewing code ... it's very hard to bite my lip and not call out every little issue, even though it would just be too much and most of the "problems" aren't actually very important. I try to impress upon people that developing good code is useful when I lead training sessions, but it's hard, especially for people who'd rather be doing data science than coding.
Code can be like that, too. You write code, and it's enough to convince you that it does what you think it does. But someone else reading it can sometimes see the holes that you don't see.
Assuming that person X writes bug-free code is bad, whether X is "you", "me", or "that co-worker".
I don't have to be better than the PR author. I just have to be decently good, and be a different person. That's enough.
And of course I'm looking for bugs. That's the absolute first thing to look for!
Now, true, if I can help the author be able to see their own bugs (asking questions, maybe), then that's probably better than me just lecturing them. Teach them how to think so that they can see it themselves next time.
https://mobile.twitter.com/sebjilke/status/10364067943591731...
(I believe TFA is Russian, so read the Eastern European graph)
Hilariously, the first graph is titled "Objective distribution" (of criticism phrasing). That's not how language works...
Good code reviews spot real bugs and provide solutions and explanations that help the owner to improve their branch. It's completely constructive and feels like going out of your way to way to help them and paying attention to what is meaningful -- the vibe should be a bit like "doorman at a posh hotel providing excellent service", so not disrespectful at all. Also, if you're nitpicking, it's a sign of lack of focus...
I was taking a graduate-level introduction to mathematical logic. In the class we learned about a bunch of different propositional axiomatic proof systems, and then we proved properties like soundness and completeness for them. The class was organized around class participation: everyone was required to do at least one proof at the board in front of the class and every session consisted almost entirely of students doing proofs at the board.
I'd been out of school, working, for eight years before grad school and I found this all very intense and unfamiliar. I did my required work at the board, choosing an easy problem, but otherwise mostly sat in the back and watched. There was, however, a cohort of very intense aspiring logicians who sat in the front row and took deep interest in every proof.
One day, a student went up with a particularly challenging problem. From the start it was clear that he was taking an unorthodox approach to the proof. The front row went nuts, interjecting and kibitzing on practically every line, telling the guy different things that he should do instead of whatever it was that he was doing. The guy was a good sport and tried to justify everything and answer all the criticism, but it seemed like he might not get through the proof before the class ended.
The prof who had been watching quietly from the side of the class, let it go on for a while, then stepped out and quieted everyone.
"I need you to remember," he said to the front row, "that we are the audience. It is not our job to prove this theorem; it is this gentleman's job. Our job as the audience is to listen to him, and at the end, decide if we are convinced. He may do things differently that you would have done, but that doesn't matter, as long as in the end we are convinced."
That was twenty-three years ago, but it stuck with me. In programming, there are often many different ways that a problem could be solved, but there is almost never One True Way™, even though there are often many voices that would like to convince you that their way is the right one. My job as a reviewer of code, or really anything, is not to try to get the other person to do what I would have done, but to decide if I'm convinced that the way that they've chosen is good enough, and, if not, to ask for changes that would convince me.
Sure, sometimes a PR is so bad that it needs to be completely rewritten, but in my experience that's rare, and in that case the feedback should probably be given in person and in private or in a friendly, sympathetic group.
Yay! you found an edge case that breaks a piece of code that will never get hit while the other million and 1 edge cases that haven't been found are still out there. Yet you drag it out from the dark and show it off for all to see like some damn trophy. Yay! look I am great I found one more thing wrong than the other guy. No matter that we are all swimming in a sea of wrong and every one is completely clueless. Only slightly less clueless than those who don't even realise they don't have a clue.
It follows from a simple argument - every programmer and every company has a different view of 'good'. I've seen it, I've worked in many. Just as every religion cannot be right, every weird view on correct programming style cannot be correct.
This is objectively, mathematically, incorrect. There are optimal solutions, and for some/many problems, there is one optimal solution.
I always look at code reviews or just reading / talking about each other's code as a way for us to just get on the same page about how we do things. Make everyone's life easier. "Hey man when we're looping through that thing here is how we usually do it." "Ok cool, I'll do that." Or maybe we have a discussion on why I did it differently this time and how we might address that.
Much of the time I don't "care" or feel strongly what the change or answer is. If we're all doing the same thing / spot a bug sooner, it's just easier for everyone.
But it seems code reviews to some are this sort of battle / proving ground / have a lot of elements of status. I don't get that...