Hacker News Reader

Comment by sarchertech | Hacker News Reader

After 20 years of doing this, I’m convinced that required PR reviews aren’t worth the cost.

In the thousands of pull requests I’ve merged across many companies, I have never once had a reviewer catch a major bug (a bug that is severe enough that if discovered after hours, would require an oncall engineer to push a hot fix rather than wait for the normal deployment process to fix it).

I’ve pushed a few major bugs to production, but I’ve never had a PR reviewer catch one.

I’ve had reviewers make excellent suggestions, but it’s almost never anything that really matters. Certainly not worth all the time I’ve spent on the process.

That being said, I’m certainly not against collaboration, but I think required PR reviews aren’t the way to do it.

View on HN

The point of code reviews isn’t to catch bugs. It’s for someone else on the team to read your code and make sure they can understand it. If no one else on your team can understand your code, you shouldn’t be committing it to the repository.

Maybe. But then, sure, I can understand the code you wrote - on a syntactic/operational level. This adds Foos to bar instead of baz, and makes Quux do extra Frob() call. Whatever, that's stupid stuff below junior level. What would actually matter is for me to understand why you're doing this, what it all means. Which I won't, because you're doing some code for symbolic transformation of equations for optimizing some process, and I'm doing data exchange between our backend and a million one-off proprietary industrial formats, and we only see each other on a team call once a week.

I'm exaggerating, but only a little. Point is, in a deep project you may have domain-specialized parts, and those specialties don't overlap well. Like, ideally I'd take you aside for an hour to explain the 101 of the math you're doing and the context surrounding the change, but if neither you nor me have the time, that PR is getting a +2 from me on the "no stupid shit being done, looks legit code-wise; assuming you know your domain and this makes sense" basis.

To do a good job on code review, reviewer must understand the challenge and how it is best to address it better than the programmer who implemented it. Necessarily!

To do an adequate job, reviewer must understand the task at least equivalently well.

The above is possible, and probably even desirable, but it adds a massive overhead in developer time for any non-trivial change. Enforcing good code review can approximately double development costs for the company, and inevitably creates a lot of programming & architecture work—nearly equivalent to any other programming & architecture work, minus the time spent hitting keyboard keys—for engineers, who might not exactly enjoy it.

As a result, it is very rare, and most reviews just tick boxes and enforce taste.

I personally hate to review substantial implementations. It is work minus the fun part. During work, I get to come up with a solution and bring it to life. Instead, during review I have to devise a solution only to use it as a reference point to assess another’s solution. This can also cause review suggestions that are difficult to enact (if my solution is sufficiently different and I think better than the original, that’s a lot of work to redo).

HN moment. I’ve never seen in practice that someone says ”I don’t understand it” and the author says ”good point, I will simplify it”.

Rather, the opposite. I often saw people make unnecessary complex or large PRs that were too much workload to review, leading the reviewer to approve, on the grounds of ”seems like you know what you’re doing and tbh I don’t have half a day to review this properly”.

Code review is a ritual. If you ask why we have it people will give you hypothetical answers more often than concrete examples. Personally I’m a proponent of opt-in CRs, ie ask for a second pair of eyes when your spidey senses tell you.

Our juniors write horribly complex code that senior devs have to ask to simplify. This happens all the time. And the juniors simplify and thank us for teaching and mentoring. It’s a big reason we do reviews. So we can control how dirty the code is before merging and so we can grow each other with constructive feedback. Sometimes it’s also just “LGTM” if nothing smells.

90% of comments in my team’s PRs come with suggestions that can be applied with a click (we use GitLab). It requires almost no effort to apply suggestions and it’s often not much extra work for reviewers to explain and suggest a concrete change.

I agree that reviews should be used pragmatically.

Get (or create) better colleagues. It's usually pretty easy to identify if people are approving pull requests that they don't understand. Pull them aside and have a professional talk about what a pull request review is. People want to do good, but you have to make it clear that you value their opinion.

If you treat the people around you as valuable collaborators instead of pawns to be played to fulfill your processes, your appreciation for reviews will transform. Remember that it's their work too.

> I often saw people make unnecessary complex or large PRs that were too much workload to review, leading the reviewer to approve, on the grounds of ”seems like you know what you’re doing and tbh I don’t have half a day to review this properly”

That just seems like company wide apathy to me. Obviously you have to make an effort to read the code, but there are lots of ways developers can overcomplicate things because they were excited to try a pattern or clever solution. It doesn't make them bad devs, it's just an easy trap to fall into.

These should not pass a code review just because the code "works." It's totally acceptable to say "we're not gonna understand this in 3 months the way it's written, we need to make this simpler" and give some suggestions. And usually (if you're working with people that care about the workload they make for others) they will stop after a few reviews that point this out.

We've done this at our company and it's helped us immensely. Recognizing whether the code is unnecessarily complex or the problem is inherently complex is part of it, though.

I feel if you ask 5 people what "the point" of codes review is, you'd get 6 different answers.

And a 7th complaining about the formatting of the question.

I watched pre merge code reviews become a requirement in the industry and catching bugs was almost always the #1 reason given.

The times I've seen a 2nd set of eyes really help with the understandability of code, it was almost always collaboration before or while the code was being written.

I would estimate something like 1 out of 100 PR reviews I've seen in my life were really focussed on improving understandability.

Wow someone who finally has this same unpopular opinion as I do. I'm a huge fan of review-optional PRs. Let it be up to the author to make that call and if it were really important to enforce it would be more foolproof to do so with automation.

Unfortunately every time I've proposed this it's received like it's sacrilegious but nobody could tell me why PR reviews are really necessary to be required.

The most ironic part is that I once caught a production-breaking bug in a PR while at FAANG and the author pushed back. Ultimately I decided it wasn't worth the argument and just let it go through. Unsurprisingly, it broke production but we fixed it very quickly after we were all finally aligned that it was actually a problem.

I'll bite.

To catch stupid mistakes like an extra file, an accidental debug flag, a missing compiler hint that has to be added to migration scripts etc.

To ensure someone who doesn't quite understand the difference between dev and production build pipelines doesn't break it.

To ensure a certain direction is being followed when numerous contractors are working on the code. For example a vague consistency in API designs, API param names, ordering, etc.

To check obvious misunderstandings by juniors and new hires.

To nix architect astronauts before their 'elegant' solution for saving a string to a database in 500 lines gets added.

To check the code is actually trying to solve the ticket instead of a wrong interpretation of the ticket.

To get introduced to parts of the codebase you haven't worked on much.

But as with anything you get from it what you put in.

None of those are good reasons why PR reviews are necessary. They are examples of things that it's theoretically possible a PR review might catch. But there's no information there about how likely those things are to be caught.

Without that absolutely critical information, no cost benefit analysis is possible.

In my experience across many companies, PR reviews almost never catch any of those bugs or bring any of those benefits.

Well, I catch those things.

Now you have some information.

If you worked with me it would be worth doing mandatory PRs.

One trick, and I'm not being sarcastic, is to read every line. Even if you don't understand the change as a whole that catches things.

Another trick, and again not sarcastic this is genuine advice. Read every line of your own PRs before you submit them. It's surprising how much I catch doing that. Same with git commits. It's also noticeable which of your colleagues don't do this as their PRs are the ones with obvious mistakes.

All of this is much easier and more effective with multiple PRs per day, and breaking bigger tickets into smaller one.

If you're constantly doing big, multi-day commits you're doing development wrong. You lose a lot of the benefits of source control and the tooling around it.

I still do big commits sometimes, especially when refactoring, but even then it's very important to keep those big commits focused on a particular change. If I find bugs or tweak a feature I've been meaning to change, I try and make a new branch and push that to main and then rebase my feature branch off main. Or cherry pick them out before the refactor PR.

> Read every line of your own PRs before you submit them

I do. Everyone should. I’m also a fan of small focused PRs.

> If you worked with me it would be worth doing mandatory PRs.

Given the number of PRs I’ve merged and the number of mistakes that reviewers have caught, I think it’s very unlikely that you’d catch those things at a frequency high enough to justify the cost.

I’m not doubting that you can find those things or that you have found them. But again I have merged thousands of PRs with hundreds of reviewers across numerous companies in multiple languages and I have never had a reviewer catch a major bug.

That’s a large enough sample size with no effect at all, that I’m going to need hard evidence to make me believe that people are finding these things at a high enough rate to justify the cost.

>Unfortunately every time I've proposed this it's received like it's sacrilegious but nobody could tell me why PR reviews are really necessary to be required.

Obvious signs of cargoculting in my opinion.

Required PR reviews means that if someone steals your credentials, or kidnaps your child, you can't get something into production that steals all the money without someone else somewhere else having to push a button also.

It's the two-person rule, the two nuclear keyswitches.

This is definitely not why PR reviews are required. Most companies don't really know why they require them, but I've definitely never heard one say it was because they were afraid of malicious code from stolen credentials.

There's so many other ways you can inject malicious code with stolen credentials that doesn't require a PR in every production environment I've ever worked in. There's much lower hanging fruit that leaves far fewer footprints.

Unfortunately for compliance reasons PRs are required.

Funny part is that not even in highly regulated markets.

ISO270001 or SOC2 are pretty much something every software company will have to do.

Yes, this is why we have required PR reviews at my company. It is to meet compliance controls.

We recently talked about not requiring reviews for people in L5 and above levels but ultimately got shut down due to compliance.

SOC2 doesn't require code reviews. SOC2 is just a certification that you are following your own internal controls. There's nothing that says required PR reviews have to be one of your internal controls. That's just a common control that companies use.

I would argue that "common control that companies use" falls under "industry standard" and I would say it would make it harder to pass certification without PR reviews documented on GitHub or something alike. So it does not require but everyone expects you to do so :)

The reason that this is common is that a company hires a SOC2 consultant who tells them that PR reviews are required despite that fact that this is a complete fabrication.

Locking yourself into an enormously expensive process with no evidence of its efficacy just because you don't want read up on the process yourself or push back on a misinformed auditor is a terrible business decision.

Curious because I am not familiar: are PRs required or are PR reviews required?

Well "Peer Review" or "Code Review" is required - pull requests are easiest way to have it all documented with current state of art tooling. Otherwise you have to come up with some other way to document that for purpose of the audit.

I agree with you. If you give each dev a kind of sand-box to "own" within a project they'll learn to find their own bugs, write both simple and robust code, lots of param checking — grow as an engineer that way.

Allowing anyone to promote anything to production without any other eyes on it is problematic. Not realizing this is extremely telling.

The presumed claim that no one at the company benefited from a second set of eyes is amazing, too.

>Allowing anyone to promote anything to production without any other eyes on it is problematic.

In my experience the people who are promoting things to production that shouldn't be will find a way to do it. They'll either wear down the people who want to stop it, or they'll find someone else to approve it who doesn't know why it shouldn't be approved or doesn't care.

My hypothesis is that requiring any 2nd random engineer in the company to approve production code doesn't provide enough value to justify the cost.

There may be other controls that are worth the cost.

However, our industry has been shipping software for a long time without this requirement, and I've seen no evidence that the practice has saved money, reduced the number of bugs, or improved software quality by any other metric. I think it's time we examine the practice instead of taking it on faith that it's a net benefit.

>Not realizing this is extremely telling.

Nice way of saying, I don't agree with you so I must be an idiot.

but there isn't actually a second set of eyes because the second set of eyes you're thinking about is complaining about formatting or slamming the approve button without actually looking

The fact that there is a PR review process in place, makes commiters try harder. And that's good!

Or try less because they have to spend time doing pr reviews

Yes, same for QA sometimes.. dev sets bar lower as the QA can test it. Just makes a bunch of back and forth. And when stuff breaks nobody feels responsible.

> I have never once had a reviewer catch a major bug

Just in 2024, I've had three or four caught[0] (and caught a couple myself on the project I have to PR review myself because no-one else understands/wants to touch that system.) I've also caught a couple that would have required a hotfix[1] without being a five-alarm alert "things are down".

[0] including some subtle concurrency bugs

[1] e.g. reporting systems for moderation and support

Reply on news.ycombinator.com