Hacker News Reader

Comment by backoncemore | Hacker News Reader

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.

View on HN

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."

Uh, whose life is being ruined?

I took "ruin developers' lives" to refer to helping get team members fired, but maybe I'm giving the author the benefit of the doubt.

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.

Strange, I’m on a team rn with people who just approve everything.

I’m craving some constructive feedback.

It's much easier to ask someone to be critical than it is to ask someone to go easy on you.

Not to be rude - but... have you asked for it?

I don’t think this is rude.

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

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?

Maybe you just write incredible code the first time, every time? Can't discount the possibility!

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?

Depends on the language. Strongly typed languages like Rust and F#, even Typescript to some degree, yes. Most of the time if it compiles it works.

I don’t think this is me, haha

The sweet spot is somewhere in the middle. Too laissez-faire and you end up with a ball of mud.

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 a security engineer.

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.

Hm, we've discussed doing more smaller concept reviews. Doing "first try" reviews sounds like a good alternative.

For two decades I wrote professionally and there were no code reviews at all. I had no problem with that.

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.

The code review was preloaded.

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.

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.

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.

* in my experience, of course.

Yeah this can be quite frustrating.

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.

IMO the absolute worst is excessive criticism while missing a serious business logic or functionality bug.

In my opinion the first priority of code review should be to prevent mistakes in the code at hand

> 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.

Oh yeah I agree, when I said mistakes in the code at hand I was considering a buisness logic bug/failure to 100% be in bound.

> excessive amount of criticism on working code

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.

If code reviews are centered around style the entire org is fucked.

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.)

> Because it creates technical debt

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.

This is why it's better to phrase the review points as questions, e.g.: "Why is this done in this way, ? Wouldn't x be more efficient ?"

Reply on news.ycombinator.com