4 ms·

> just get rid of code review You’ve lost me there and will never get me back. I can’t abide any workflow that doesn’t put code review as top priority, regardl

> just get rid of code review You’ve lost me there and will never get me back. I can’t abide any workflow that doesn’t put code review as top priority, regardless of remote or in person, sync or async.

lamontcg 4y ago
A lot of code review is needless nitpicking that doesn't help to produce bug free software and mostly provides friction. Particularly with senior devs reviewing other senior devs work. I don't think I'd say that code review needs to be abolished, but it often needs to be put on a bit of a diet. And I've seen PRs where code beautification feedback has gotten so out of hand and the PR is now 10% code fixes and 90% unrelated cleanup in the class/module/whatever that I can no longer determine if its safe to merge or not and that no regressions were introduced by the code review process itself.
- orwin 4y ago
  > I don't think I'd say that code review needs to be abolished, but it often needs to be put on a bit of a diet. To clarify: You mean whole team meeting for code-review needs a diet? Because i agree. I actually think code review is always positive in pair programming situation, and often a waste of time in a team meeting (even with smallish teams). Maybe include the new dev during the code review just to witness (and ask questions later for his enboarding).
  - lamontcg 4y ago
    Uh no, I mean the doctrine of code review in general needs to be put on a diet. I'm definitely opposed to everyone sitting around synchronously nitpicking everyone else's code in enforced meetings, but I wasn't considering that. Onboarding new devs is where code review and pair programming is obviously useful. And honestly I've seen it go both ways, where onboarding a new dev was an opportunity for the old veterans to learn some new tricks. If code review for new devs is just beating them into submission with your own code standards for the sake of your own code standards, then it becomes more of an ego-flexing exercise. But day-to-day it can get very repetitive and if I'm looking at a PR by a dev who has been around for a long time, doing work which is repetitive work that we've already beaten to death how to do it all correctly, I'm going to give it a very thin skim--that would probably absolutely horrify people who deify code review--before just approving it. And most of the regressions that I've seen shipped went through code review and everyone missed the edge condition, and done 100 more times everyone would have missed it again, and nobody would have seen the deficiency in the tests. Donald Rumsfeld's unknown-unknowns. This obviously doesn't apply at all to externally-contributed code to open source software, because you can't really trust external contributors to think through all the edge conditions, you have to assume they're operating in "just-fix-my-bug" mentality and the fix may not be correct or may not be (adequately) tested. That's where you need to be very defensive and need to have your "steward of the codebase" hat on. But for teammates that you've worked with for years, everyone should have their own hat pretty well-developed and its good for velocity and reduction of frustration to trust them a lot more.
    - shakezula 4y ago
      > looking at a PR by a dev who has been around for a long time This is precisely the case where I think you need code review the most, because in my experience, time makes people complacent. Code review is peer review, the key agent of the scientific method. I just can’t abide a process that doesn’t include peer review. > Donald Rumsfeld's unknown-unknowns. are an excellent reason to have others question your code. Code review is a culture issue. If you have people who are so unfamiliar with code as to not be able to review it, then that’s a clear knowledge silo that seems perfectly addressable by _code review_. It’s not about the code, it’s about the knowledge transfer and peer enforcement.
      - incrudible 4y ago
        > Code review is peer review, the key agent of the scientific method. I just can’t abide a process that doesn’t include peer review. The key agent of the scientific method is making predictions and comparing them to reality. In other words, you should test if your code actually does what it should do. If so, peer review does not necessarily offer any net benefit. If you absolutely insist on code review, then you do not belong on the team I am envisioning, which is totally fine. Like I said, the approach has obvious risks, but that is often a tradeoff worth making.
      - lamontcg 4y ago
        Well, as someone who had a decade of experience with the codebase I absolutely submitted PRs which were largely unreviewable because nobody else had that level of experience. My safety net wasn't code review it was writing tests, and actually firing up the product and determining that it worked (which a large amount of people actually never bother doing, and I'd prefer that over code review any day). In those cases we'd do some knowledge transfer, but I can't turn someone into a veteran with decades of experience in an hour or two of code review. I'd actually be happy to do a week of knowledge transfer on the issue and treat it like a PhD dissertation defense (which is about what they were sometimes), but nobody else would want to commit that kind of time, and no manager would want to commit the team to that kind of time. And for knowledge transfer what usually works better is having more junior members of the team doing work on subsystems and guiding them through it. Even if it isn't peer review or pair programming, the iterative process of them hitting walls and asking questions is generally the best learning. That works better because they go off and commit the time to struggling with the problem, and then guidance has that platform to build on top of. And you don't understand unknown-unknows... You can't address that by code review or knowledge transfer or peer enforcement, because it is all the absolute unknowns. A healthy skepticism can help prevent risky changes, but at some point the bugs that get through are often completely out of left field that literally nobody could have foreseen. There is no perfect process that can prevent those kinds of defects.

lamontcg 4y ago

A lot of code review is needless nitpicking that doesn't help to produce bug free software and mostly provides friction. Particularly with senior devs reviewing other senior devs work. I don't think I'd say that code review needs to be abolished, but it often needs to be put on a bit of a diet. And I've seen PRs where code beautification feedback has gotten so out of hand and the PR is now 10% code fixes and 90% unrelated cleanup in the class/module/whatever that I can no longer determine if its safe to merge or not and that no regressions were introduced by the code review process itself.

orwin 4y ago
> I don't think I'd say that code review needs to be abolished, but it often needs to be put on a bit of a diet. To clarify: You mean whole team meeting for code-review needs a diet? Because i agree. I actually think code review is always positive in pair programming situation, and often a waste of time in a team meeting (even with smallish teams). Maybe include the new dev during the code review just to witness (and ask questions later for his enboarding).
- lamontcg 4y ago
  Uh no, I mean the doctrine of code review in general needs to be put on a diet. I'm definitely opposed to everyone sitting around synchronously nitpicking everyone else's code in enforced meetings, but I wasn't considering that. Onboarding new devs is where code review and pair programming is obviously useful. And honestly I've seen it go both ways, where onboarding a new dev was an opportunity for the old veterans to learn some new tricks. If code review for new devs is just beating them into submission with your own code standards for the sake of your own code standards, then it becomes more of an ego-flexing exercise. But day-to-day it can get very repetitive and if I'm looking at a PR by a dev who has been around for a long time, doing work which is repetitive work that we've already beaten to death how to do it all correctly, I'm going to give it a very thin skim--that would probably absolutely horrify people who deify code review--before just approving it. And most of the regressions that I've seen shipped went through code review and everyone missed the edge condition, and done 100 more times everyone would have missed it again, and nobody would have seen the deficiency in the tests. Donald Rumsfeld's unknown-unknowns. This obviously doesn't apply at all to externally-contributed code to open source software, because you can't really trust external contributors to think through all the edge conditions, you have to assume they're operating in "just-fix-my-bug" mentality and the fix may not be correct or may not be (adequately) tested. That's where you need to be very defensive and need to have your "steward of the codebase" hat on. But for teammates that you've worked with for years, everyone should have their own hat pretty well-developed and its good for velocity and reduction of frustration to trust them a lot more.
  - shakezula 4y ago
    > looking at a PR by a dev who has been around for a long time This is precisely the case where I think you need code review the most, because in my experience, time makes people complacent. Code review is peer review, the key agent of the scientific method. I just can’t abide a process that doesn’t include peer review. > Donald Rumsfeld's unknown-unknowns. are an excellent reason to have others question your code. Code review is a culture issue. If you have people who are so unfamiliar with code as to not be able to review it, then that’s a clear knowledge silo that seems perfectly addressable by _code review_. It’s not about the code, it’s about the knowledge transfer and peer enforcement.
    - incrudible 4y ago
      > Code review is peer review, the key agent of the scientific method. I just can’t abide a process that doesn’t include peer review. The key agent of the scientific method is making predictions and comparing them to reality. In other words, you should test if your code actually does what it should do. If so, peer review does not necessarily offer any net benefit. If you absolutely insist on code review, then you do not belong on the team I am envisioning, which is totally fine. Like I said, the approach has obvious risks, but that is often a tradeoff worth making.
    - lamontcg 4y ago
      Well, as someone who had a decade of experience with the codebase I absolutely submitted PRs which were largely unreviewable because nobody else had that level of experience. My safety net wasn't code review it was writing tests, and actually firing up the product and determining that it worked (which a large amount of people actually never bother doing, and I'd prefer that over code review any day). In those cases we'd do some knowledge transfer, but I can't turn someone into a veteran with decades of experience in an hour or two of code review. I'd actually be happy to do a week of knowledge transfer on the issue and treat it like a PhD dissertation defense (which is about what they were sometimes), but nobody else would want to commit that kind of time, and no manager would want to commit the team to that kind of time. And for knowledge transfer what usually works better is having more junior members of the team doing work on subsystems and guiding them through it. Even if it isn't peer review or pair programming, the iterative process of them hitting walls and asking questions is generally the best learning. That works better because they go off and commit the time to struggling with the problem, and then guidance has that platform to build on top of. And you don't understand unknown-unknows... You can't address that by code review or knowledge transfer or peer enforcement, because it is all the absolute unknowns. A healthy skepticism can help prevent risky changes, but at some point the bugs that get through are often completely out of left field that literally nobody could have foreseen. There is no perfect process that can prevent those kinds of defects.