4 ms·

In some environments, that is true. But the process of review can also help build shared knowledge and understanding of the changes and the code base.

MrJohz 3y ago
I'm increasingly of the opinion that this should be the primary value of code review. The goal is to make sure that multiple people working on a codebase have at least some idea of what's going on in every corner of it. Obviously that's never going to be perfect, and you won't understand code as well by reading it as by working with it, but at least having some sort of familiarity is invaluable. The other important benefit is catching stupid mistakes - formatting, missing code, > vs >=, etc - dumb mistakes that everyone makes. But ideally most of those mistakes are caught by types, tests, and linting, so the code review just makes sure that a different pair of eyes has sanity checked things. If code reviews become mainly about this sanity check, then it's often a sign that the types, tests, or linting are inadequate (or that someone is being too picky). The other problem is when reviews too often focus on the big problems - architectural discussions about how to implement a feature - in which case there's usually a need to have more architectural discussion before implementation starts.
- detourdog 3y ago
  I agree with you and the person you are agreeing with. I have to add that using analysis takes none of that away. Using analysis creates a culture of analysis tools. This also can make the revealing of performance surprises a shared learning experience from the grayest beard to the greenest horn.
  - MrJohz 3y ago
    Completely agree, analysis tools go hand in hand with code review as a learning tool. Partly, they take more responsibility away from finding the obvious bugs, and partly, like you say, they help surface problems that people might not be looking out for. I'd love it if things like performance checking tools were more commonly used.
  - ryanschneider 3y ago
    One thing I really really miss from working in person on a small team was that we did all PR reviews in person, almost like a little “thesis defense” session. This was medical adjacent signal processing software so that rigor was necessary, but it really helped make sure we all knew exactly what was changing and why, as part of the review was going over any output changes in our test corpus and making sure they were all for the better and understood _why_ an output value changed. You just can’t do that async inside a GitHub PR to the same level.
- jt2190 3y ago
  > The goal is to make sure that multiple people working on a codebase have at least some idea of what's going on in every corner of it. If that is truly the goal, shouldn’t the programmer have that information before they start writing code? > The other important benefit is catching stupid mistakes - formatting, missing code, > versus >=, etc - dumb mistakes everyone makes. I’d love this to be true, but I’ve now seen so many bugs missed in PRs because the reviewers get bogged down in “code style” feedback (which is this generation’s commas versus tabs debate I swear) that they completely miss the more severe issues. Additionally, the reviewers often only have shallow knowledge of the business requirements and can’t know whether > or >= is appropriate. > The other problem is when reviews too often focus on the big problems - in which case there’s usually a need to have more architectural discussion before implementation starts. Totally agree here. To be clear, I support the idea of feedback from other developers during the development process, but I feel that little thought is given to (a) what kind of feedback is truly valuable, (b) when the feedback should happen, and (c) who is in a good position to provide that feedback.
  - MrJohz 3y ago
    > > The goal is to make sure that multiple people working on a codebase have at least some idea of what's going on in every corner of it. > If that is truly the goal, shouldn’t the programmer have that information before they start writing code? Could you clarify that a bit? I agree that programmers would ideally start out with some knowledge of the codebases they work on, but by working on the codebases, that typically make changes and so that knowledge is quickly out of date! ;) That's the value of code review as knowledge sharing - as new things are implemented, or old parts refactored, or new technologies introduced, or even parts deleted, multiple people are involved in those changes and so gain that knowledge. EDIT: and I completely agree with the rest of your comment! Giving good feedback is a learned skill, and too often we don't put the time in to learn (or teach) it properly.
    - jt2190 3y ago
      My larger point is that a code review can only happen after the programmer has changed some code. For anything that is flagged during the review it’s fair to ask: Was this requirement known before the programmer started writing code, and if so, why was it not communicated earlier? Again, this isn’t the “fault” of the code review, it’s that there are insufficient or missing pieces of the software development process overall. Yes, having a code review is a good thing, but we often try to make it do all the things.

MrJohz 3y ago

I'm increasingly of the opinion that this should be the primary value of code review. The goal is to make sure that multiple people working on a codebase have at least some idea of what's going on in every corner of it. Obviously that's never going to be perfect, and you won't understand code as well by reading it as by working with it, but at least having some sort of familiarity is invaluable. The other important benefit is catching stupid mistakes - formatting, missing code, > vs >=, etc - dumb mistakes that everyone makes. But ideally most of those mistakes are caught by types, tests, and linting, so the code review just makes sure that a different pair of eyes has sanity checked things. If code reviews become mainly about this sanity check, then it's often a sign that the types, tests, or linting are inadequate (or that someone is being too picky). The other problem is when reviews too often focus on the big problems - architectural discussions about how to implement a feature - in which case there's usually a need to have more architectural discussion before implementation starts.

detourdog 3y ago
I agree with you and the person you are agreeing with. I have to add that using analysis takes none of that away. Using analysis creates a culture of analysis tools. This also can make the revealing of performance surprises a shared learning experience from the grayest beard to the greenest horn.
- MrJohz 3y ago
  Completely agree, analysis tools go hand in hand with code review as a learning tool. Partly, they take more responsibility away from finding the obvious bugs, and partly, like you say, they help surface problems that people might not be looking out for. I'd love it if things like performance checking tools were more commonly used.
- ryanschneider 3y ago
  One thing I really really miss from working in person on a small team was that we did all PR reviews in person, almost like a little “thesis defense” session. This was medical adjacent signal processing software so that rigor was necessary, but it really helped make sure we all knew exactly what was changing and why, as part of the review was going over any output changes in our test corpus and making sure they were all for the better and understood _why_ an output value changed. You just can’t do that async inside a GitHub PR to the same level.
jt2190 3y ago
> The goal is to make sure that multiple people working on a codebase have at least some idea of what's going on in every corner of it. If that is truly the goal, shouldn’t the programmer have that information before they start writing code? > The other important benefit is catching stupid mistakes - formatting, missing code, > versus >=, etc - dumb mistakes everyone makes. I’d love this to be true, but I’ve now seen so many bugs missed in PRs because the reviewers get bogged down in “code style” feedback (which is this generation’s commas versus tabs debate I swear) that they completely miss the more severe issues. Additionally, the reviewers often only have shallow knowledge of the business requirements and can’t know whether > or >= is appropriate. > The other problem is when reviews too often focus on the big problems - in which case there’s usually a need to have more architectural discussion before implementation starts. Totally agree here. To be clear, I support the idea of feedback from other developers during the development process, but I feel that little thought is given to (a) what kind of feedback is truly valuable, (b) when the feedback should happen, and (c) who is in a good position to provide that feedback.
- MrJohz 3y ago
  > > The goal is to make sure that multiple people working on a codebase have at least some idea of what's going on in every corner of it. > If that is truly the goal, shouldn’t the programmer have that information before they start writing code? Could you clarify that a bit? I agree that programmers would ideally start out with some knowledge of the codebases they work on, but by working on the codebases, that typically make changes and so that knowledge is quickly out of date! ;) That's the value of code review as knowledge sharing - as new things are implemented, or old parts refactored, or new technologies introduced, or even parts deleted, multiple people are involved in those changes and so gain that knowledge. EDIT: and I completely agree with the rest of your comment! Giving good feedback is a learned skill, and too often we don't put the time in to learn (or teach) it properly.
  - jt2190 3y ago
    My larger point is that a code review can only happen after the programmer has changed some code. For anything that is flagged during the review it’s fair to ask: Was this requirement known before the programmer started writing code, and if so, why was it not communicated earlier? Again, this isn’t the “fault” of the code review, it’s that there are insufficient or missing pieces of the software development process overall. Yes, having a code review is a good thing, but we often try to make it do all the things.