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

41–50 of 122 posts

Re: How to run a miserable code review

#41

Post by thegrimmest »

datavirtue wrote: post #25
SilverBirch wrote: post #21

Earlier quoted context omitted.

Totally agree it's flawed, but very often the reason it's flawed is because something has gone wrong somewhere else. Yes, it's very common for someone to do a PR and for someone else to turn around and go "Oh god, why the hell did you do it like this?!" - Fundamentally challenging the entire purpose of the code. What's happening there is the code review is just highlighting the fact you didn't have a design review. Y…

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

Re: How to run a miserable code review

#42

Post by dj_gitmo »

braza wrote: post #31

(honest question, please do not downvote): Why the tech industry still relies in the CR as a quality assurance/correctness process?

By CR do you mean meetings where everyone talks about the code? Or a github-style pull-request? I personally find meetings where we talk about a pull-request to be a poor use of time. I want to have time to read, reread, and interact with the code.

Re: How to run a miserable code review

#43

Post by BlargMcLarg »

ar_lan wrote: post #37
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…

Everything you listed are exactly what the "know-it-all" senior developer is expecting already, though. If you're doing these things before you submit your review for eyes to look at, you'll come to a point where your reviews should only go through 0-2 revisions before submission is ready. Personally, if I see a review that is >10 files that isn't explicitly a "Refactor" review, or I've been prepped ahead of time, it…

Counterpoint, most places do not teach individuals how to check in often and make their stuff smaller. Some of them don't even realize merging side branche into side branch is a valid strategy to avoid merging incomplete features into the main branch(es).

The seniors are not getting out of this scot-free when they barely make an effort to educate themselves, let alone others, or strategize ways to make this dummy-proof.

Re: How to run a miserable code review

#44

Post by jeltz »

braza wrote: post #31

(honest question, please do not downvote): Why the tech industry still relies in the CR as a quality assurance/correctness process?

Because when done in a team where people want to work together and do not have incentives discouraging that it is a really good tool for spreading knowledge and improving maintainability. So far I have only once worked at a place where the review culture was bad and that was due to issues much bigger than the code reviews (specifically a culture where a different team wrote code for another team to maintain, encouraging shoveling shit code over the fence).

Re: How to run a miserable code review

#45

Post by kiitos »

jeltz wrote: post #36
bobthepanda wrote: post #33

Earlier quoted context omitted.

YMMV but i work at a place where * code review is expected responsibility, so everyone participates in every part of it regularly, so they are also incentivized to keep the process sane * we have an auto linter and we recommend saving on fix specifically so no one argues about useless style nits * CR back and forth is measured in minutes or hours so you are not waiting days to resolve someone’s drive by comment * CR…

> * CR feedback always has a specific action item that is easy to address Then CRs are pretty much pointless. The feedback I want as a senior developer is the complex stuff and that is half of the time not easy to address. The trivial stuff I usually, but not always, spot myself when checking the code before sending it for a review.

Reviewers are responsible for not just pointing out issues, but also providing (at a minimum) some form of direction, or (more ideally) one or more explicit suggestions as to how to resolve those issues.

This is an essential component of a productive code review culture.

Re: How to run a miserable code review

#46

Post by thegrimmest »

I agree that this post outlines a miserable code review, but where I work I often see the opposite: a miserable submission. Is it wrong to expect engineers new to the project spending a few whole days (read 8-24 full hours) just reading the documentation, code, tests, project layout, etc.? This time would be invaluable to avoid reimplementing things that already exist, following existing patterns and styles, understanding where to put things, etc.

Why do people expect to come on to a multi-100k-line project and just slap out a complex feature in a few days? Of course they'll end up with many dozens of comments from senior maintainers and everyone (including POs) will be very frustrated. It's pretty easy to avoid though by just following a sane ramp-up.

Re: How to run a miserable code review

#47

Post by debacle »

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 blatantly wrong in a way that worked on their machine, or worked for specific uses cases, but would never work in production.

3. The evil gremlin code reviewer was an asshole, liked being an asshole, and didn't care how many people thought he was an asshole.

4. The antihero programmers refused to learn how to be better programmers from the evil gremlin code reviewer, and the evil gremlin programmer refused to learn how to be educative, rather than castrative, in his code reviews.

5. Rather than serving as a quality enforcing function, most code reviews were spiked by the accounts team, meaning that evil gremlin code reviewer was in a perpetual state of frustration, antihero programmers were constantly in a cycle of post-production bug fixing/optimization, and the pace of development invariably ground to a halt over time.

6. No one cared how little I cared about this problem, but it was a nice excuse to not have to talk to the accounts team, and so I spent a lot of time listening to people whine.

Re: How to run a miserable code review

#48

Post by bretthardin »

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…

Everything is a people problem - even code review. SWE who choose to not grow (by refusing to learn) are going to create awful culture.

Re: How to run a miserable code review

#50

Post by Capricorn2481 »

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?
Post reply on HN