Hacker News Reader

Comment by AnimalMuppet | Hacker News Reader

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".)

View on HN

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.

Reply on news.ycombinator.com