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.
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.
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.
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.
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
OP is Short-form false-dichotomy. Thank you for this perfect response.
100% agree. It's as if you read the first sentence out of my mind. Thanks.
Similar to #3:
- having someone more experienced in that subdomain catch problematic parts and inconsistencies with existing code.
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.
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:
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.