5 ms·

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

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

loeg 1y ago
> 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.
mugsie 1y ago
> 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
- monkeyelite 1y ago
  Does it make the code better? The best projects are the ones with the most review l?
Capricorn2481 1y ago
> 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.
- monkeyelite 1y ago
  > 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.
  - skydhash 1y ago
    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.
cbsks 1y ago
> 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.
- monkeyelite 1y ago
  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.
  - skydhash 1y ago
    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.

loeg 1y ago

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

mugsie 1y ago

> 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

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

Capricorn2481 1y ago

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

monkeyelite 1y ago
> 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.
- skydhash 1y ago
  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.

cbsks 1y ago

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

monkeyelite 1y ago
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.
- skydhash 1y ago
  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.