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

51–60 of 122 posts

Re: How to run a miserable code review

#51

Post by agentultra »

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?

There are a lot of qualitative reasons: it helps spread knowledge of changes in the code, it helps maintain a consistent style, shares knowledge, etc.

However when it comes to quality assurance and correctness (if these terms collectively mean, preventing defects) then there's little evidence that it is an effective practice [0]. If the changes proposed are less than a couple hundred lines of difference and the reviewer is only reading one every couple of hours there's small but significant chance that they might catch an error. Humans are simply bad at this task.

[0] https://sail.cs.queensu.ca/data/pdfs/EMSE_AnEmpiricalStudyOf...

Why does the tech industry still rely on CR for this purpose? Probably because running empirical studies is time consuming and expensive. Instead we rely on the intuitions, experiences, and feelings of people, advice we get from others, etc.

Re: How to run a miserable code review

#52

Post by ctvo »

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 it actually catches issues.

Your unit tests are never good enough. Your integration tests won't match your production environment. Your canaries only test the happy path. If your goal is full CI/CD, some changes will make it through that will impact at least a subset of your customers in production without being caught.

A good code review process utilizes a larger portion of the team's understanding of a system, not just your own, to help catch some of these issues. This understanding could be system, product, or inter-team dependencies that you will never fully codify into an automated process.

It also socializes best practices, help folks learn new patterns and improve as software engineers.

Re: How to run a miserable code review

#53

Post by sanderjd »

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…

If the code review process is (through your supportive wisdom) pushing the people you work with to follow those practices, then I would argue it is actually a pretty good process. Those are all very good things to do, that a good code review culture should definitely be pushing people toward.

For instance, let's imagine this without the code review process and your subsequent advice on how to make it go better: People would just be merging in the 20 file PRs, sight unseen, without validating a strategy beforehand, without really knowing how to approach the problem, with the code just written and slapped together without tweaking and refactoring, and without any self-review using a critical eye to catch low-hanging fruit like leaving in console.logs calls. I think the "flawed" code review process seems like a much better outcome!

Re: How to run a miserable code review

#54

Post by chefandy »

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…

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 have the guidance to avoid this problem in the first place, that's on the senior developer(s). These are on-the-job-learned skills, and the reason junior developers make less money is because they need guidance from seniors figuring out how all of that stuff works. Secondly, if they did have explicit guidance, have been advised to tighten things up a few times, yet can't swing it, the senior developer needs to be a senior developer and empathically help them work through whatever strategic block is causing the problem. Finally, if the junior has been told many times and not cleaned up their practices, the senior developer isn't doing them any favors by playing the role of rankled, imperious elder-- the person is likely not cut out for the role, or needs to spend more time learning, maybe as an intern. It needs to be addressed with their manager so the right person can fill that position.

If senior developers don't want to do that then they should work some place that doesn't hire juniors.

Re: How to run a miserable code review

#55

Post by kxrm »

fuzzy2 wrote: post #11
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…

What, those are rookie numbers! I opened a pull request 13 business days ago. I even rebased it multiple times when conflicting code was otherwise merged. There’s been no feedback whatsoever. Everyone even remotely involved is now on vacation. (It’s not like I care either, the project sucks anyway.)

The best is when the only possible reviewer is your Engineering Manager because you have no other engineers in your domain on the team.

Re: How to run a miserable code review

#56

Post by ar_lan »

BlargMcLarg wrote: post #43
ar_lan wrote: post #37

Earlier quoted context omitted.

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-pro…

I agree the onus is to establish a clear culture, guidelines, and mentorship to newbies and junior developers in this regard.

Almost everybody who started their career probably had their first code review torn to shreds. I'm not saying that's the right way to do it. But I will say, just like when a junior developer comes in thinking "well it works, so it's right", there are almost always other considerations than just the happy path.

A many-file, massive review might be 100% technically correct and flawless, and it's still going to take reviewers a lot longer to review than if they split it into 2-3.

--

> Merging side branch into side branch is a valid strategy

Completely agree! This is a fantastic strategy. You're not affecting the main branch, and generally people reviewing that specific branch can have the context of what your change is doing.

Re: How to run a miserable code review

#57

Post by duxup »

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.

Re: How to run a miserable code review

#58

Post by candiddevmike »

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.

Re: How to run a miserable code review

#59

Post by ar_lan »

chefandy wrote: post #54
ar_lan wrote: post #37

Earlier quoted context omitted.

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…

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 wholly wrong, I try to whiteboard it out with them instead. I hope I've always come across nicer/a good mentor from this. I'm sure someone has disagreed :)

--

All in all I completely agree. Senior devs + managers need to set the stage, expectations, and provide the necessary mentorship/utilities needed to accomplish.

Also, a pet peeve of mine - one of my first teams I was on, a dev always commented on style issues. It got to the point where numerous junior devs complained and finally some other engineer stepped in and said "I don't disagree with your style comments, but you'd save yourself the headache if you just wrote a linter to catch that automatically." It's a pretty clear example in my mind of someone who finds self-importance in their voice being shown on each code review, when a simple utility would save everyone the headache.

Re: How to run a miserable code review

#60

Post by candiddevmike »

ashtonbaker wrote: post #22
marktangotango wrote: post #4

I once worked at a place where about 5 of 15 developers sat on crucible all day fellating each other on code reviews. Anytime the rest of the developers would have code reviewed they'd be met with a long list of required changes that where "standards" the 5 had agreed on, and never communicated out side of their own comments in the tool. And the "standards" changed often, and the changes where never communicated eith…

What makes you call your current review culture “sane”? I’m not sure I have seen that yet in my career.

Prioritize code reviews over development work. Have SLAs. If you are assigned a CR, get to a good stopping place, pause your work, do the CR, and resume. Close CRs that have become stale due to submitter abandonment.
Post reply on HN