Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

How to run a miserable code review

badsoftwareadvice.substack.com

Post reply on HN
Print view View original

61–70 of 122 posts

Re: How to run a miserable code review

#61

Post by s__s »

duxup wrote: post #57
notjustanymike wrote: post #2

So many missed opportunities for a small business. A code review should never be done in person; instead all communication should happen asynchronously through passive aggressive messages left in GitHub. If you're on bi-weekly sprints, the review should be left to age for a minimum of 5 business days before any feedback is provided. Never write resolvable comments, but rather leave ambiguous musings of how a block of…

Code reviews seem like a hellscape of "could do X". Like man it's code, there's an infinite amount of "could do" with an infinite amount of contexts. I found google's advice to be pretty good https://google.github.io/eng-practices/review/ while they give a lot of good advice / suggestions, they also make a point that there aren't a lot of hard stops and generally if the code works and isn't horrendous you let it go.

If the code works let it go? That’s an incredibly low bar to set. Unless you’re working on an all-star team, that approach will quickly get your codebase in a really bad place with incredible amounts of technical debt.

Re: How to run a miserable code review

#62

Post by chefandy »

ar_lan wrote: post #59
chefandy wrote: post #54

Earlier quoted context omitted.

I agree that the coding expectations are fine-- and I'm not saying you do this-- but conveying annoyance through pedantic, overly nitpicky, or snarky junior code reviews is a management and mentorship failure. In any field, someone consistently acting like a know-it-all is indulging their own emotional shortcomings under the guise of enforcing good practices. Firstly, if this is a brand new junior and they didn't hav…

> but conveying annoyance through pedantic, overly nitpicky, or snarky junior code reviews is a management and mentorship failure Completely agree. If I get a chance, I almost always try to have a conversation (Zoom, in person) instead of writing large walls of text. It's definitely discouraging to, really anybody, to see your review get slammed by someone. Usually, if I see a common pattern or something is just whol…

Yes, I agree with all of that. Additionally, it's important that management understands that seniors need the time to actually do these things. You don't just get whatever percent less efficiency with junior developers, it takes time from your senior developers doing their work to help the juniors along.

Re: How to run a miserable code review

#63

Post by withinboredom »

cjfd wrote: post #18
agentultra wrote: post #15

Earlier quoted context omitted.

Minimum 5 days hurt. Too real. If there’s one way to destroy productivity and morale it’s to leave a vague comment on a review and not come back to it for a few days.

5 days is very quick. I have an open pull request that has been open for 6 months or thereabouts.... :-( No, I am not suggesting this as a good practice.... Actually, I would say that a pull request should be reviewed in 24 hours. Pull requests made on Friday may wait until Monday.

I have an open PR from my first week. Not a single review. I’ve merged entire features since then…

Re: How to run a miserable code review

#64

Post by mbwgh »

Over time, a programmer gathers a personal set of quality heuristics that has worked for them. A code review is an opportunity to sanity-check your changes by running your code against another person's set of heuristics.

There will be legitimate warnings and false-positives. You can gain some insights into how other people perceive and model the world, and in the process learn something, from both.

It's sad how a lot of people (including in the comments here) choose waste this opportunity by taking offense, and assuming that other people expressing their thoughts (which is an error-prone, lossy convertion process) are generally not well-intentioned.

Re: How to run a miserable code review

#65

Post by [deleted] »

Capricorn2481 wrote: post #50
PartiallyTyped wrote: post #19

How to make your colleagues miserable 101. 1. open a cr, 2. have your inexperienced friend approve it, 3. have the lead eng add 15 comments they caught, 4. merge the code anyway despite it breaking the core feature that’s being merged in parallel because you can’t just follow the design of the person who thought everything through and decided you knew better, 5. waste 8 more hours of your lead while they pair program…

How does a company even allow a person that does this to work there?

[deleted]

Re: How to run a miserable code review

#67

Post by atq2119 »

thegrimmest wrote: post #41
datavirtue wrote: post #25

Earlier quoted context omitted.

[flagged]

I dunno mate, I think adults should be able to handle being chortled at. You're responsible for your own emotional state. If you've done something laughable (or even if you haven't), then don't be surprised when people laugh. Taking it personally is optional and not recommended.

I agree that people shouldn't be too thin-skinned, it's still important to stay civil and supportive.

Being able to clearly and unambiguously convey that a change sucks while not hurting the other person's feeling is a useful skill to practice. (It's okay to fail at it, too, as long as you're aware that that's almost always a weakness.)

Re: How to run a miserable code review

#68

Post by atq2119 »

candiddevmike wrote: post #58
debacle wrote: post #47

A lot of this is hyperbolic whining. Back when I worked for other people, I had the luxury (not a luxury) of being the mediator between the evil gremlin code reviewer and the wayward antihero programmers. Takeaways: 1. The evil gremlin code reviewer was almost always right, and was almost unequivocally the smartest programmer at the company. 2. Our antihero programmers not only tended to be wrong, they tended to be b…

Fire the gremlin, it's never worth picking an IC over the team morale. Folks can learn to be better programmers, assholes will always be assholes.

The problem with this advice is that, at least in this story, if we're taking it at face value, the antihero programmers didn't learn.

Re: How to run a miserable code review

#69

Post by withinboredom »

schwartzworld wrote: post #17

Code review is a flawed process, especially for junior engineers. You spend time and effort, maybe days of work, getting this to work, and then some know-it-all leaves a bunch of nit-picky comments or tells you you're doing it wrong. I tell most of the juniors I work with to work defensively against this using a few strategies: - smaller PRs. Break the work up any way you can. Make small tickets or submit your PRs wi…

I always tell juniors to remember that it is their code until it gets merged in. Feel free to tell people “no” and why (“addressing” a comment doesn’t necessarily mean the code has to change). If the reviewer doesn’t like it, they can do it themselves after it gets merged in or create a PR to the PR. There are only a few cases where they can actually get blocked: leaking PII, security issues, and egregiously bad code (like writing something 5x times instead of using a loop).

Anyway, if I’m reviewing and I see some improvements, I usually open a PR to the PR with my suggestions. I sometimes get halfway through it and realize why it is the way it is and never even leave a comment (and will defend the PR if someone else does a low-effort suggestion).

Re: How to run a miserable code review

#70

Post by duxup »

s__s wrote: post #61
duxup wrote: post #57

Earlier quoted context omitted.

Code reviews seem like a hellscape of "could do X". Like man it's code, there's an infinite amount of "could do" with an infinite amount of contexts. I found google's advice to be pretty good https://google.github.io/eng-practices/review/ while they give a lot of good advice / suggestions, they also make a point that there aren't a lot of hard stops and generally if the code works and isn't horrendous you let it go.

If the code works let it go? That’s an incredibly low bar to set. Unless you’re working on an all-star team, that approach will quickly get your codebase in a really bad place with incredible amounts of technical debt.

I think you have to read their process to be clear. They don't want to hold up things over style quibbles and etc. It is something they're ok with talking about, but not every aspect that could be improved is a hard stop.
Post reply on HN