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.

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.

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.

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.

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

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?

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.

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.

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.

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.

I work on a team that runs following a high trust and domain ownership model. When I joined our process was "write some code, test it, be confident in the code, commit it to trunk". A number of years ago we worked our way through "Clean Code" in a reading group and decided to try out code reviews. Each week one of us would present our recent work and the team would tear it apart line by line, and in the process we developed a better understanding of our group philosophy around style and practices.

When we switched to Git we decided that we liked code reviews and wanted to make them part of merging to develop. Depending on the content the reviews are deeper or more superficial. Many times even when I review the code of somebody making changes in their own subject matter domain I have found bugs that simply needed another set of eyes to catch.

I like code reviews and in the context I use them find that they are a complement to trust, not a substitute.

I feel I work on a high performance team and code reviews are a joy. When I submit my code for review it's an opportunity to share my work with my peers, potentially explain my thought process and my work process and we get to learn from one another, and when I get code to review it's the opposite, I get to see how my coworkers think and tackle problems.

I have never once felt like a code review did anything to erode trust and I don't see how a code review could slow down a work process. Using git, as soon as I submit code for review I begin work on the next task and if I need anything from the branch I submitted for review, I just merge that into the new branch. If changes are requested I make the changes and then merge again.

Code reviews are among the best ways of ensuring that code remains consistent and that everyone is learning from everyone else. That's why in my team all pull requests are reviewed by two people including one junior developer.

On my team, the level of scrutiny a merge request receives depends on the author and the functionality. MRs from a principal or lead engineer tend to get rubber-stamped at a higher rate, though, to your point, many of us will explicitly call out code that we would like to have reviewed in more detail. If a more junior level developer submits a merge request, it is highly likely their code will get a close review.

I'm not sure I agree with your premise that code reviews are the product of a culture with a lack of trust and ownership. I think, in fact, submitting to a review process is a sign of ownership -- it's saying "hey, the quality of the code we produce as a team is important to me, and I know I make mistakes."

I‘m called a principal engineer in my organization (I know that can mean everything or nothing) I like to use code review requests and sometimes gates also to put a fence in front of my ego. Also this is a great way of sharing knowledge. I had great sessions where questions about X and then answers verbally or in text suddenly put stuff into different perspective. Sometimes I realize that my solution might be to complicated or an more generic, performan, (paste fancy adjective to describe your code here) is easy to achieve.

I trust the people whose code I review to be competent developers. But everyone occasionally makes typos, misreads requirements, has blind spots, etc. Even if the creator is the expert and the reviewer is there to catch only obvious mistakes, that still has value.

> But everyone occasionally makes typos, misreads requirements, has blind spots, etc.

Case in point: OP used the word "effect" when he should have used "affect".

Just like pudding, there's always room for a code review.

This is scary to read. We use code reviews for changes and we’ve found that our more junior team members can bring a point of view our more senior team members can’t and vice versa. It’s one of the best parts of working in code for me.

With that said, code reviews only work, in my experience, if team members are strong communicators. Understanding each other, empathizing with issues, and being collaborative rather than tearing people down is critical.

Code reviews are a “we” activity not a “your” and “my” activity.

Yes on trust. However… I'm pretty darn experienced in what I do these days, and every day I still write a bit of crappy code, usually as I come to an understanding over time of the current problem. WTFs come from code written six months ago as well as yesterday. If someone can point out an issue early, it saves everyone time. So, I welcome it.

This used to be common sense in the few companies I worked at around 2012.

People just pushed code. We occasionally held some review sessions to go over code and exchange comments / ideas about what might be wrong with this or that code and how to prevent this kind of mistake in the future.

But starting from around 2016, I could never ever find a company that does not have mandetory code reviews for every single commit, and this bothers me tremendously.

It's one of the reasons I don't want to work at any programming company anymore.

The really good programmers I know don't look positively at code reviews.

If you need code reviews, ask for it. If not having mandetory reviews results in a huge stream of bugs, maybe you should hire better programmers instead of cheap ones.

Where do we find all of those better programmers?

There's a flip between when the feedback is super, super helpful (consensus building, as you say), to a moderate inconvenience to screwing up your estimates because you seem to always post a PR right after the reviewers have started some new thing.

Then farther down that continuum someone is busting your chops for some stupid reason that, after you get back from lunch or coffee, you realize is completely valid (or even gives you an idea for an enhancement) and okay you change how a conditional works or add some more tests.

It makes me wonder if in a corporate setting PRs would be better treated as pseudorandom audits, based on a random factor and a weight for automated linting (so people don't learn to game the linter, which can be some of the worst code you ever see.)

A related issue is that most tools make it pretty difficult to look at and interact with old PRs. Feedback loops have some slop, and while it's better that you get the feedback as you are writing the code, fast follow often works as well, and I think we'd be better as a group if you could request revisions on a recently landed PR after the fact. This is mostly okay but you really need to fix your variable names, conditional block, and you missed a boundary condition over here that might not break today, but there are already stories in the backlog that will break it for sure.

On my team we have one other engineer, no matter your or their official position, just do a quick glance over for obvious issues. This does not prevent real bugs from getting merged in, the test cases are supposedly for that but even those have some slippage.

I see 2 clear problems with this ideology:

1) There is too much knowledge to fit inside one person's head. Cooperation is the keystone of success.

2) Your team will grow. You will need to share this knowledge somehow before the trust can be given

But in defense of this view point: code reviews shouldn't be the opposite of what you're talking about.

I trust my teammates to catch any mistakes I've made. I'm human and therefore fallible. It's going to happen.

I trust mistakes they've made are for the same reasons.

What you're trusting is your teammates are top tier 1% every time and bordering on being super human.

I'm trusting that my team has my back and we're in it together.