Hmm… this seems to take as a premise that the main point of the code review is to suggest changes to the code before it is suitable for merging.
I don’t think that’s the main benefit. If you’re worried about “inventing tasks for each other” then you can just have a rule that “if anyone makes a suggestion during code review that you disagree with, it suffices to say ‘no’ and if they feel strongly about it then they can file an issue in the tracker”
Okay, so what benefits do I claim still exist? In some sense the benefit is accidental: by mandating code reviews, you ensure that every piece of code has been seen by at least two people: author and reviewer. This helps people learn about the code base far faster than if they only ever look at code they wrote themselves, and reduces bus factor (Alice is on vacation, no one else knows this code). The record of the discussion can help archeologists later when trying to understand “why is it done this way???” and the discussion itself (even if the record is lost) means at least one other developer now understands the business requirement this bit of code was meant to address.
I’ve never been a manager but I suppose most of these benefits would be nearly invisible: YOU certainly know what the business requirement was; the benefits from 2 developers on a team of 30 understanding it, instead of just 1 in 30, are presumably very subtle. Perhaps they are too small to be worth it.
But if that’s what you think then you should say so; instead you never mention the knowledge spreading aspects at all, which makes me suspect you didn’t even consider it.
a rule that “if anyone makes a suggestion during code review that you disagree with, it suffices to say ‘no’ and if they feel strongly about it then they can file an issue in the tracker”
"In some sense the benefit is accidental: by mandating code reviews, you ensure that every piece of code has been seen by at least two people: author and reviewer. This helps people learn about the code base far faster than if they only ever look at code they wrote themselves"
This is fine, but this simply becomes a kind of pair programming, and actual (in real time) pair programming might be more targeted.
The kind of pair programming that I see most often, at most companies, amounts to:
1. some software developer writes some code
2. one of the core developers then reviews the code (on a team of 30, this might be the most senior 3 developers who do the reviews)
As such, the code reviews tend to both overburden the more experienced developers, and also concentrate all knowledge in them.
For spreading knowledge among peers, I think pair programming would be the most targeted approach, although what you suggest would also work fine.
Hmm… this seems to take as a premise that the main point of the code review is to suggest changes to the code before it is suitable for merging.
I don’t think that’s the main benefit. If you’re worried about “inventing tasks for each other” then you can just have a rule that “if anyone makes a suggestion during code review that you disagree with, it suffices to say ‘no’ and if they feel strongly about it then they can file an issue in the tracker”
Okay, so what benefits do I claim still exist? In some sense the benefit is accidental: by mandating code reviews, you ensure that every piece of code has been seen by at least two people: author and reviewer. This helps people learn about the code base far faster than if they only ever look at code they wrote themselves, and reduces bus factor (Alice is on vacation, no one else knows this code). The record of the discussion can help archeologists later when trying to understand “why is it done this way???” and the discussion itself (even if the record is lost) means at least one other developer now understands the business requirement this bit of code was meant to address.
I’ve never been a manager but I suppose most of these benefits would be nearly invisible: YOU certainly know what the business requirement was; the benefits from 2 developers on a team of 30 understanding it, instead of just 1 in 30, are presumably very subtle. Perhaps they are too small to be worth it.
But if that’s what you think then you should say so; instead you never mention the knowledge spreading aspects at all, which makes me suspect you didn’t even consider it.
Thoughts?
I like this rule:
a rule that “if anyone makes a suggestion during code review that you disagree with, it suffices to say ‘no’ and if they feel strongly about it then they can file an issue in the tracker”
About this:
"In some sense the benefit is accidental: by mandating code reviews, you ensure that every piece of code has been seen by at least two people: author and reviewer. This helps people learn about the code base far faster than if they only ever look at code they wrote themselves"
This is fine, but this simply becomes a kind of pair programming, and actual (in real time) pair programming might be more targeted.
The kind of pair programming that I see most often, at most companies, amounts to:
1. some software developer writes some code
2. one of the core developers then reviews the code (on a team of 30, this might be the most senior 3 developers who do the reviews)
As such, the code reviews tend to both overburden the more experienced developers, and also concentrate all knowledge in them.
For spreading knowledge among peers, I think pair programming would be the most targeted approach, although what you suggest would also work fine.
Okay with that context, everything you wrote makes more sense to me now