Comment by atomashpolskiy | Hacker News Reader

Comment by atomashpolskiy | Hacker News Reader

The problem is not with code reviews but with how adversarial they have become. Instead of making the best attempt to comprehend and embrace the author's style and intention and finding compelling arguments for the question "why this code is probably fine and should be merged as-is?" people nit on all kinds of stuff, mostly highly subjective. Along the way they don't actually catch that many problems or bugs but instead new problems and bugs are introduced while implementing their nits. All surrounded by a lot of noise that has to be dealt with in an already stressful multitasking environment. I agree with other posters in this thread that most people can't code review for crap and have very perverted idea about code review aims and goals. Code reviews as they are practiced now only serve to destroy trust, velocity and team morale. As someone noted elsewhere in this thread, if it's me (directly or indirectly) who's going to be held accountable for a deficiency in my code, whether it has been reviewed or not, why do I have to adjust my code to someone's arbitrary criticism? Common sense tells me that not requiring to have my code reviewed at all times and, by doing so, stressing my personal responsibility for the things I do, would actually increase my sense of ownership and keep me on my toes, more likely leading to better coded/tested changes, wouldn't it? I.e. it is not about blindly trusting all code that I produce but rather trusting in my ability to judge and make final decisions: to request code reviews or not, to address comments or not, to do my job as I see fit or not.

> The problem is not with code reviews but with how ... they have become.

Perhaps "banal" is the word you are meant to use.

There's a saying about that a 10 line PR will get 10 comments, but a 1000 line PR will get a "looks good!".

Of course this is not really what is happening. A 10 line PR can be completely unintelligible and a 1000 line PR might be a joy to read.

Once the code base has become a ball of mud, no one really bothers to review anything properly - it's going to take as long as writing it yourself. So perhaps, we get picky about some worn out code style point (incorrect indent etc) just to show we are alive.

> Once the code base has become a ball of mud, no one really bothers to review anything properly

This is the standard code review experience I've had across companies I've worked at. It's just theatre, bad and irritating theatre.

> Instead of making the best attempt to comprehend and embrace the author's style and intention and finding compelling arguments for the question "why this code is probably fine and should be merged as-is?" ...

I don't think we should take it as given that the goal should be to approve the code in the originally written form. In some sense, I think if you're in an environment where you're going to be doing PRs for every change anyway, if your code is consistently perfect when you cut the PR, then probably you've cut it too late?

What if instead we frame PRs as the basis for an artifact-based discussion about how to do something. The code needs to exist in a complete enough form to provide a concrete basis for discussion. But if you see your reviewer as a resource and collaborator, who might have ideas or insights that you didn't, then ask for those ideas or insights a bit earlier, before the code is polished.

The best PRs aren't the ones that catch a bug. The best PRs offer some criticism or question or suggestion that allows you to reframe an abstraction or refactor something to be simpler, more flexible, more testable, more readable, faster, whatever, and from which the original author learns something. Your code might not have had a bug before, but that doesn't mean it cannot and should not be improved.

Circling back to 'trust' though, this approach does require a degree of trust. The reviewer can't be focused on nits and gotchas; more minor things are inevitable if our colleagues pull us in sooner, but we have to trust that they'll get sorted out.

> The problem is not with code reviews but with how adversarial they have become.

I've never really seen that. What do you think is the reason for people thinking that's the thing to do?

Incentives set up a race to the bottom.

I worked in an environment that was like Raycast and later adopted gatekeeping code reviews. If the code reviewer wasn't happy you couldn't merge. This gives the reviewer power to have the author shape the code the way the reviewer prefers at little cost to the reviewer. And of course the authors often respond by doing likewise when asked to review code. My approach as a reviewer was to provide feedback without blocking merges, but this alone is not enough to incentivize reciprocal behavior.

There was a guy at work who would always complain about numbers that weren't given names as #defines regardless of whether it made sense or not (Magic Numbers!). My co-worker asked him why he was so picky about it. Apparently because someone did that to him.

Reply on news.ycombinator.com