Hacker News Reader

Comment by monkeyelite | Hacker News Reader

Code reviews kill velocity - introduce context switching, and are make work, it feels like you’re doing something to make a PR etc but your not.

The context it makes the most sense is accepting code from strangers in a low trust environment.

The alternative to trying to prevent mistakes is making it easy to find and correct them. Run CI on code after it’s been merged and send out emails if it’s failed. At the end of a day produce a summary of changes and review them asynchronously. Use QA, test environments, etc.

View on HN

> Code reviews kill velocity

This feels like a strange sense of priorities which would be satirised in a New Yorker/Far Side single-panel comic: “Sure, my mistake brought down the business and killed a dozen people, but I’m not sure you appreciate how fast I did it”.

Code should be correct and efficient. Monkeys banging their heads against a keyboard may produce code fast, but it will be brittle and you’ll have to pay the cost for it later. Of course, too many people view “later” as “when I’m no longer here and it’s no longer my problem”, which is why most of the world’s software feels like it’s held together with spit.

> would be satirised in a New Yorker/Far Side single-panel comic:

Thanks for taking my experience and comment seriously and challenging your preconceptions.

> Code should be correct and efficient.

When it ships to customers. The goal is to find the bugs before then. Having a stable branch can be accomplished in many ways besides gating each merge with a review.

Do you have any studies to show how effective synchronous code review is in preventing mistakes? If they are such a good idea why not do 2 or 3?

> Thanks for taking my experience and comment seriously and challenging your preconceptions.

I apologise if my comment read as mean. I wanted to make the joke and it may have overshadowed the point.

> Do you have any studies to show how effective synchronous code review is in preventing mistakes?

I could’ve been clearer. I’m not advocating for code reviews, I’m advocating for not placing “velocity” so high on the list of priorities.

> If they are such a good idea why not do 2 or 3?

This argument doesn‘t really make sense, though. You’ve probably heard the expression “measure twice, cut once”—you don’t keep measuring over and over, you do it just enough to ensure it’s right.

> I’m not advocating for code reviews.

Well my comment is against synchronous code reviews. So we are not in disagreement.

> you do it just enough to ensure it’s right.

I agree. Each layer of review etc is a cost and has benefits. You want to pick an appropriate level.

> Thanks for taking my experience and comment seriously and challenging your preconception

I'm honestly curious what you're experience level is? I've never known a developer with more than a couple years of experience valuing velocity over financial stability

The up-front cost of code review can be easily be tripled or quadrupled when it’s distributed over several weeks after the fact in the form of unplanned work, each instance of which incurs its own cost of context switching, as well as the cost of potential rework.

The purpose of such a review is a deliberate bottleneck in the earlier stage of development to stop it becoming a much larger bottleneck further down the line. Blocking one PR is a lot cheaper than blocking an entire release, and having a human in the loop there can ensure the change is in alignment in terms of architecture and engineering practices.

CI/CD isn’t the only way to do it but shifting left is generally beneficial even with the most archaic processes.

> The up-front cost of code review can be easily be tripled or quadrupled when it’s distributed over several weeks

You’re taking a more extreme position than the one I’m stating. You can review every day or every hour if you want.

> a deliberate bottleneck in the earlier stage

Wouldn’t it be better if we could catch bugs AND avoid the bottleneck? That’s the vision. Good intentions may disagree about how to accomplish that.

To catch bugs and avoid the bottleneck that is code review, you’re just moving the bottleneck to some point after merging the code, not before.

Like it or not you still have to stop what you’re doing to identify a bug and then fix it, which takes time away from planned feature work. You’re not optimising anything, you’re just adding fragility to the process.

As I said before, an issue localised to a PR in review blocks one person. An issue that has spread to staging or prod blocks the entire team.

> Code reviews kill velocity

Yes, they kill your velocity. However, the velocity of a team can be massively increased by shipping small things a lot more often.

Stable branches that sit around for weeks are the real velocity killer, and make things a lot more risky on deployment.

I agree with all of that - no contradiction.

> Code reviews kill velocity - introduce context switching, and are make work

This is the same point three times, and I don't agree with it. This is like saying tests kill velocity, there's nothing high velocity about introducing bugs to your code base.

Everything introduces context switching, there's nothing special about code reviews that makes it worse than answering emails, but I'm not going to ignore an important email because of "context switching."

Everyone makes mistakes, code reviews are a way to catch those. They can also spread out the knowledge of the code base to multiple people. This is really important at small companies.

CI is great, but I have yet to see a good CI tool that catches the things I do.

> This is the same point three times

No it isn’t. Fake work, synchronization, and context switching are all separate problems.

> code reviews are a way to catch those

I said you can do reviews - but there is no reason to stop work to do them.

Why not require two or three reviews if they are so helpful at finding mistakes?

I agree everyone makes mistakes - that’s why I would design a process around fixing mistakes, not screening for perfection.

How many times have you gone back to address review comments and introduced a regression because you no longer have the context in your head?

> Why not require two or three reviews if they are so helpful at finding mistakes?

Diminishing returns, of course. I have worked places where two reviews were required and it was not especially more burdensome than one, though.

I catch so many major errors in code review ~every day that it's bizarre to me that someone is advocating for zero code review.

> Why not require two or three reviews if they are so helpful at finding mistakes?

Places do? a lot of opensource projects have the concept of dual reviews, and a lot of code bases have CODEOWNERS to ensure the people with the context review the code, so you could have 5-10 reviewers if you do a large PR

Does it make the code better? The best projects are the ones with the most review l?

> No it isn’t. Fake work, synchronization, and context switching are all separate problems

Context switching is a problem because it...kills velocity. Fake work is a problem because it kills velocity. You're saying it's time that could be better spent elsewhere, but trying to make it sound wider. I disagree with the premise.

Synchronization is a new word, unrelated to what you originally wrote.

> How many times have you gone back to address review comments and introduced a regression because you no longer have the context in your head?

Never? I am not unable to code in a branch after a few days away from it. If I were, I would want reviews for sure! Maybe you have had reviews where people are suggesting large, unnecessary structural changes, which I agree would be a waste of time. We're just looking for bug fixes and acceptably readable code. I wouldn't want reviewers opining on a new architecture they read about that morning.

> Synchronization is a new word, unrelated to what you originally wrote.

I believe you can figure it out.

> Never?

Ok well I’m trying to talk to people who have that problem. Because I and my team do.

I guess they say never, because if you have descriptive commit messages and write good PR description, it's easy to regain the context surrounding the change. It's all about communication, both to others and your future self.

> Why not require two or three reviews if they are so helpful at finding mistakes?

For secure software, e.g. ASIL-D, you will absolutely have a minimum 2 reviewers. And that’s just for the development branch. Merging to a release branch requires additional sign offs from the release manager, safety manager, and QA.

By design the process slows down “velocity”, but it definitely increases code quality and reduces bugs.

Once again let me reframe the mindset. Trying to get a perfect change where you anticipate every possible thing that will go wrong beforehand is impossible - or at least extremely costly. The alternative is to spend your effort on making it easy to find and fix problems after.

You are not anticipating every possible bugs. It's mostly a learning experience for you and the team if it's done correctly. Someone may proposes another approach, highlight certain aspects that needs to be done "right" (definition may vary), let you know possible pitfalls, etc... It's not always LGTM.

Reply on news.ycombinator.com