4 ms·

Code review doesn't have a single purpose. Finding code that is hard to maintain is one of those, and and an important one, but certainly not the only one, and

by thayne 3mo ago

Code review doesn't have a single purpose. Finding code that is hard to maintain is one of those, and and an important one, but certainly not the only one, and I'm not sure it is even the most important one. Other purposes include: - a safety check to ensure that if a developer (or AI) goes rogue, it is more difficult to merge malicious code - a second perspective from someone who isn't as close to the problem and might see a better way to do things, or problems that the original developer missed - in some cases having someone more familiar with other parts of the system look at it who can tell if it won't interact well with something else - ensuring there is at least one other person familiar with the code - a learning opportunity. The author can learn from feedback from the review, and the reviewer can learn from the code in the change. Especially important when the author and reviwer have differing seniority. When I mentor a new employee, I add them as a reviewer to all my PRs so they can see how I do things, and review all their PRs so I can provide guidance. And sometimes I even learn things from them! - yes, catching bugs, although this should not be the primary mechanism for that, and I agree is not the most important reason. It is especially important for security and performance bugs though, as those are harder to catch with automated testing.

urig 3mo ago
100% agree. It's as if you read the first sentence out of my mind. Thanks.
a4isms 3mo ago
Another—very old—rationale: People write code differently when they know that it will be reviewed by people who will not only comment on it, but also form long-term impressions of the submitter's competence and fit based on the code that is reviewed.
- freedomben 3mo ago
  This is the big one IMHO. I've been part of teams where there was zero code review, and where the code reviews were intense, and for sure people write much better code (generally speaking) when they know it will be reviewed. We all do it, even if subconsciously. I do advocate a balance though. Ridiculous code reviews tend to slow the process down immensely, which is good for some things but bad for others. Finding a good balance is super important IMHO
- dotancohen 3mo ago
  I've always felt that this is and advantage to open source software. The vast majority of open source software that I've bothered to look at the code for used best practices, was reasonably secure, and was above all maintainable. The bespoke projects that I've worked on at various companies? Complete spaghetti messes almost all of them.
  - skydhash 3mo ago
    Pretty much. I use OpenBSD and the basic stance is that you need to look at the code of the system and the various software in ports. Because the only way to get timely support is you helping yourself and then the community will help you. And if you find some hackish code, there’s generally a good reason it’s that way.
levl289 3mo ago
OP is Short-form false-dichotomy. Thank you for this perfect response.
ivanjermakov 3mo ago
Similar to #3: - having someone more experienced in that subdomain catch problematic parts and inconsistencies with existing code.
- eptcyka 3mo ago
  And having someone less experienced can also bring perspectives and criticism to assumptions that can and sometimes should be challenged.
threethirtytwo 3mo ago
I agree it doesn’t have a single purpose but you didn’t refute the claim about a primary purpose. One purpose to rule them all.
boie0025 3mo ago
I agree with all of this, I read into the "single purpose" of understanding the code and complaining about what you don't understand implying that if you understand it, you will be able to point out and comment on things that are wrong /foolish/unsafe/etc after understanding it. From that perspective on the OP, it makes sense to me. Particularly with regard to modularity and factoring; once I understand all of a gigantic PR I will have modeled it in my mind and will begin to either see that it will be maintainable, or will be a total nightmare one day... or somewhere in between.
- boie0025 3mo ago
  After a re-read, I realized the claim wasn't "single purpose", it was "primary purpose", in which case this makes even more sense to me. I guess everything else comes from understanding what's in a given PR. It is difficult to find bugs in code that you don't understand, and it's difficult to understand code that doesn't follow convention, etc. I think I've worked this way and just not thought about it from this perspective. I review a lot of code, and what I generally do is fire up my editor in the relevant repo and follow along. If there's a method call to outside of the PR, depending on what it is and what I know about it, I'll pull that up in my editor and review there to make sure I understand what's happening. That understanding is where the comments come from. Maybe "I understand this and it's right" or "I understand this, and it seems wrong because [something]" or "I do not understand this because [whatever]" etc. Maybe "primary purpose" isn't perfect.. perhaps "overarching goal" or similar.. :shipit:
janpeuker 3mo ago
Exactly - Readability Review [1] at Google was tedious but I generally liked the idea of having 2 separate discussions with different goals 1) https://abseil.io/resources/swe-book/html/ch03.html#readability_standardized_mentorship_thr https://abseil.io/resources/swe-book/html/ch03.html#readabil...

by thayne 3mo ago

urig 3mo ago

100% agree. It's as if you read the first sentence out of my mind. Thanks.

a4isms 3mo ago

Another—very old—rationale: People write code differently when they know that it will be reviewed by people who will not only comment on it, but also form long-term impressions of the submitter's competence and fit based on the code that is reviewed.

freedomben 3mo ago
This is the big one IMHO. I've been part of teams where there was zero code review, and where the code reviews were intense, and for sure people write much better code (generally speaking) when they know it will be reviewed. We all do it, even if subconsciously. I do advocate a balance though. Ridiculous code reviews tend to slow the process down immensely, which is good for some things but bad for others. Finding a good balance is super important IMHO
dotancohen 3mo ago
I've always felt that this is and advantage to open source software. The vast majority of open source software that I've bothered to look at the code for used best practices, was reasonably secure, and was above all maintainable. The bespoke projects that I've worked on at various companies? Complete spaghetti messes almost all of them.
- skydhash 3mo ago
  Pretty much. I use OpenBSD and the basic stance is that you need to look at the code of the system and the various software in ports. Because the only way to get timely support is you helping yourself and then the community will help you. And if you find some hackish code, there’s generally a good reason it’s that way.

levl289 3mo ago

OP is Short-form false-dichotomy. Thank you for this perfect response.

ivanjermakov 3mo ago

Similar to #3: - having someone more experienced in that subdomain catch problematic parts and inconsistencies with existing code.

eptcyka 3mo ago
And having someone less experienced can also bring perspectives and criticism to assumptions that can and sometimes should be challenged.

threethirtytwo 3mo ago

I agree it doesn’t have a single purpose but you didn’t refute the claim about a primary purpose. One purpose to rule them all.

boie0025 3mo ago

I agree with all of this, I read into the "single purpose" of understanding the code and complaining about what you don't understand implying that if you understand it, you will be able to point out and comment on things that are wrong /foolish/unsafe/etc after understanding it. From that perspective on the OP, it makes sense to me. Particularly with regard to modularity and factoring; once I understand all of a gigantic PR I will have modeled it in my mind and will begin to either see that it will be maintainable, or will be a total nightmare one day... or somewhere in between.

boie0025 3mo ago
After a re-read, I realized the claim wasn't "single purpose", it was "primary purpose", in which case this makes even more sense to me. I guess everything else comes from understanding what's in a given PR. It is difficult to find bugs in code that you don't understand, and it's difficult to understand code that doesn't follow convention, etc. I think I've worked this way and just not thought about it from this perspective. I review a lot of code, and what I generally do is fire up my editor in the relevant repo and follow along. If there's a method call to outside of the PR, depending on what it is and what I know about it, I'll pull that up in my editor and review there to make sure I understand what's happening. That understanding is where the comments come from. Maybe "I understand this and it's right" or "I understand this, and it seems wrong because [something]" or "I do not understand this because [whatever]" etc. Maybe "primary purpose" isn't perfect.. perhaps "overarching goal" or similar.. :shipit:

janpeuker 3mo ago

Exactly - Readability Review [1] at Google was tedious but I generally liked the idea of having 2 separate discussions with different goals 1) https://abseil.io/resources/swe-book/html/ch03.html#readability_standardized_mentorship_thr https://abseil.io/resources/swe-book/html/ch03.html#readabil...