Hacker News Reader

Comment by quadrifoliate | Hacker News Reader

The popularity this sentiment on HN terrifies me.

Please don't do this. I keep finding rubber stamp code reviews (or none at all) while diving into codebases for where bugs were introduced, and 9 times out of 10, it's someone who didn't bother to get proper reviews for their code because "it's just something slowing me down". I can assure you that you have bugs and usability issues in your code, not just "linter issues". Everyone does.

If your code reviews are becoming rubber stamps, that's an indication that you need to slow down and teach other team members about the system, or hire smarter people who can understand your reviews more quickly. Don't respond by getting rid of the review process as a knee-jerk response to these sorts of organizational constraints.

View on HN

I disagree.

If you have a practice that isn't effective (e.g. code reviews that aren't preventing bugs), doubling-down is almost never the correct solution.

Code reviews are like unit tests; they're helpful under the right conditions. Mostly on the tricky bits or when a dev is working in an unfamiliar area.

It's a common fallacy for folks to believe that if a little of something is good, then a lot of it must be better. I've been at places with 100% code test coverage or requiring EVERY SINGLE PULL REQUEST to be reviewed. It's obnoxious, and it doesn't do shit to prevent bugs. Instead it causes folks to burn out on the practice and write crap unit tests or stamp "LGTM" on every PR they're forced to evaluate. People have a limited capacity for cognitive engagement and wasting that on piddly crap means they can't spend it on the important bits.

If your code reviews aren't catching bugs, the correct answer is to fix the code reviews, not to stop doing them.

I mean, it's possible that code reviews are worthless. But enough people have found value in them, across enough organizations, enough code styles, and enough decades, that it seems really hard to conclude that. It seems more probable that the team that finds them useless is doing them wrong. (Yeah, I know, No True Scotsman. Still, the first response should be "maybe our team is doing them wrong, and can we fix that?", rather than "let's stop doing them".)

I don't disagree with you at all, but I'm not sure I've been a part of an organization that does it right. Beyond the stuff like style, conventions, and obvious bugs/problems, it's challenging to provide meaningful insight to someone that has many more hours of experience in a particular area of a codebase more than you do. I know I personally hold back from calling out things that I find to be subjective -- even if I think I would have done it differently -- simply because I'm often unsure about the things I don't know.

Plus as a peer reviewer, you're often in the same part of the sprint cycle as they are - meaning your mind is already juggling a lot of things about your own code. It's tough to "come up for air" and then immediately dive into someone else's code changes with enough mental capacity to provide a truly useful review.

It's easy to slip into the rubber stamping mentality when you generally trust the ability of a particular contributor.

"Beyond the stuff like style, conventions, and obvious bugs/problems ..." Catching the obvious issues is IMO one of the main benefits of never skipping code reviews. Even trivial changes often have those (at least when I write them) and I've most likely saved many hours of debugging by a reviewer catching stuff like that. But this does require more than simple rubber stamping. I encourage my reviewers to read my code with the assumption that I make errors, basically looking for the bug that's fairly likely to be there.

The other thing I think is easy to do is simply see if you understand the code. If the reviewer struggle to do understand what's going on, chances are that other people will struggle as well. In this case, your lack of knowledge is an asset, the original author is likely to not see how it looks for someone not as well versed with the problem.

This - last year I broke something because I'd changed a query in a file completely unrelated to the ticket I was working on, and the guy who did the review treated code review as a rubber-stamp - he completely ignored the fact I'd accidentally committed a change to payment processing code in a patch to tighten up company number validation.

Even if code reviewers only made sure the files changed made sense, the reviews would be worth it.

You assume here that people only work on the area of the codebase which they are most familiar with. I think a sign of a healthy team is when people are contributing to other parts of the code and not getting stuck in a rut where each area of code only has one maintainer/gatekeeper.

Also, for less experienced members, reviewing the changes of more experienced people is a great way to learn if you properly sit down and understand the change (why it’s being done, why it’s being done this way) rather than just skimming through. In addition to catching mistakes, this is useful to check that code is actually comprehensible to people other than the author and a good time to request more/better comments if necessary.

I’m not assuming that at all. When you’re at a company that has $Xmm lines of code floating around in its repositories, it’s unrealistic to assume you will be able to cross functionally train all developers on all code. Of course you try to aim for that, but you’ll inevitably find yourself in situations where you don’t.

Not all developers on all code, but N developers on any given piece of code, where N should be greater than 1. Why were you asked to do the review? Is N equal to 1 so there is nobody besides the author who has the expertise needed to do the review? That's a good opportunity to start the process of raising N to 2. Is it because N is equal to 2, but the other person is out on vacation? Better start working on getting N to 3 then!

> stuck in a rut where each area of code only has one maintainer/gatekeeper.

Why do you assume it's a rut? It might be an organizational liability but it can be a great while it's working. It's common folklore that design by committee leads to poor outcomes, or at best average outcomes completed slowly.

Why assume that we need to build code by consensus or a series of interchangeable cogs?

> In addition to catching mistakes, this is useful to check that code is actually comprehensible to people other than the author and a good time to request more/better comments if necessary.

The most convoluted code I ever worked with were in teams that treated code review as secret cow. It happened twice. The best was in teams without code review, but with strong ownership.

The issue is, I think, that code review is myoptic. It ensures surface level readability and does very little for actual structure. And I think it gave people false confidence and made them less willing to refactor.

You should ask about the stuff you don't understand or aren't sure about. Code review is a communication tool. If you're unsure about how something works, someone else is going to be unsure about that later. It probably needs to be clarified in code or documentation.

This is where I've found the most value. It's so much more effective to communicate about implementation after the developer has taken a first stab at the problem.

"a lot of organizations do X" doesn't seem like a great reason to do X. A lot of organizations use microservices.

I don't think this is doubling down so much as singling down.

The problem with unit tests and code reviews is that they ultimately aren't sexy. There's no way to do them that destroys the integrity of the system. Especially when a large percentage of the code written is passable. Yes, reviewing and testing code that works and is bug free is a bit of a waste of time. But you won't know what code isn't working and bug free unless you test and review.

And then there's the fact that most of what we do is not that important. If my code is buggy or inefficient or a third thing, the worst thing that happens is a few people frown while I get it sorted. If you knew that if your code had a bug in it, it would definitely kill 5 - 10 people every time it is run, you'd treat review and testing with the utmost importance because the cost of committing buggy code is way too high.

Testing and review becomes exactly as important as your product.

Code reviews are not meant to prevent bugs. They are for making sure an individual's effort is good enough for the team to assume further maintenance of that piece of code. That almost never means that the code must be bug-free, it means that other people than the author can follow the thought processes expressed in the code. If that leads to other people finding shallow bugs, more power to them. But that doesn't mean that bug prevention is (or should be) the primary goal of code reviews.

First and foremost, code reviews are for enforcing team standards, such as testing coverage, documentation, and adhering to scope. "Our code reviews aren't preventing bugs" is not a useful metric in that sense. If you're relying on code reviews to prevent bugs, you have other problems in your development process. If I spot a bug during code review, my first question would be "why wasn't this bug found by the automated test suite?"

> But that doesn't mean that bug prevention is (or should be) the primary goal of code reviews.

Totally agree and well said. In my experience the mentality that code review should catch bugs puts unnecessary pressure on the reviewer which leads to people not wanting to review code, because what if they missed a bug.

Every place I've worked has required reviews on pull requests. Are there any notable tech companies that don't?

My colleagues sometimes find non-trivial errors in my code, and I find errors in theirs. Maybe we're just not as smart as the people you work with.

Velocity vs correctness is a trade-off. Stringent review, testing, and ownership requirements can easily slow down developers by an order of magnitude. Which is often the right call, but not always.

Velocity is only measured by the value you introduce. Incorrect code decreases value.

Been on both camps of this argument. Now tend to favor that responsible seniors should be allowed a fast lane. It depends very much on your team dynamics. Seniors can be very cowboy so some of them actually need even more review than others.

Have been in some teams with very high junior/senior ratio where a junior would need a whole day to review a small thing a senior implemented in an hour, often because understanding the context leads to hundred rabbit holes of more new information, not the small diff itself. Add the one week that PR was waiting for someone to even notice it needed review and you realize the frustration. Vice versa a junior would take a week to implement something a senior can review in 15mins.

Do the math and you realize the senior will quickly get blocked.

Slowing down and teaching others in that situation is a as you say the right thing to do. But that also saturates after a while. You can only pair program x number of hours per day before getting exhausted, especially for a beginner this can feel like a lot of pressure to ingest so much knowledge at that pace. They need to slow down and get in their own zone as well. In the meantime, backlog is on fire with tasks that need to be released yesterday.

Hire smarter people? Yes please, would love to do that, where do I call? “Smarter” doesn’t always help either, the reality is that you often end up with domain experts within some areas of the product, even if you try to keep everyone on board.

I have some sad news for you. Code reviews don't eliminate bugs.

Some times, they actually hinder the process of fixing bugs.

Reply on news.ycombinator.com