What I find to be maybe the single most important part of code review is knowledge transfer.<p>Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time. There's no being blindsided like "this whole system I depend on is gone" like I had happen at far more siloed places I've worked.<p>Beyond that, it gives a forum to ask questions about how things work to further build understanding. On a high functioning team, every developer should have at least a modest understanding of the entire system, including parts they never touch.<p>Another <i>important</i> feature is just the institutional knowledge check. For instance recently I made a small change to a table and a coworker pointed out that there was a microservice I wasn't considering that wrote to that table that would break (yes, sharing tables is bad design, unrelated). I had no idea this microservice existed let alone had access to this table. The institutional knowledge check here though prevented a larger issue and potential data cleanup situation.
We even find ourselves creating PRs in situations where the code is going to be merged immediately anyway, and tagging other devs, just so they have a convenient way to see what got merged and why. So people don't lose track of what is in the codebase.
My attitude has always been that code review is best thought of as the gate where code goes from being owned by the author to being owned by the team or project. The code I'm reviewing is not your code, it is code that is about to become our code.<p>Maintainability is a major factor in that, of course.
This just makes reviewers and authors lazier.<p>The purpose of code review is multi-faceted. Hard to maintain? Yes. Might have bugs? Yes. Can be done simpler/cleaner? Yes. Is in line with project code style? Yes. Get someone else to also understand the code? Yes. Onboard junior team member? Yes. Sanity check design decisions? Yes.<p>This flippant note is mostly more self-justification for being a lazy code reviewer.
True - the biggest thing I want to catch in an MR is "will this change lead us onto a part thatnl is uglier, buggier, less maintenanable".<p>People will generally copy and follow existing patterns, so for example if you let somebody add a new internal date time format, then soon your codebase will bifurcate and there'll be multiple inconsistent versions roaming around.<p>The other stuff (minor bugs, overly verbose code) can easily be fixed. Paradigm rot cannot.
Sure, ensuring maintainability is one of the benefits of code reviews, but I think it is a bold claim to say that's the solo purpose. For example, code reviews is also a tool that allows teams to get inform of the changes in the code and share responsibility of the whole code base.
The author is a mathematician, so when he says “it is not in general possible to find bugs by examining the code” he does not mean it is completely impossible to find bugs. He means only that it is not possible to find all bugs or even any particular bug.
The best writing on this is the "agent principal-agent" problem, which correctly frames the problem of agents and code review in terms of trust.<p>This is why the solutions for high-trust environments (small teams) and low-trust environments (big companies, open source projects) will be different.<p><a href="https://crawshaw.io/blog/agent-principal-agent" rel="nofollow">https://crawshaw.io/blog/agent-principal-agent</a>
Thanks, this articulates something that I've been struggling to put a finger on. You can't review agent generated code the same way you would review a PR, someone needs to fine comb it to make sure everything is fine. And doing that for something like 100,000 lines of code over a few weeks just doesn't sound realistic to me.
It’s probably important to define what sort of code review you are talking about when making broad claims about it.<p>GitHub style asynchronous pull request review with inline comments is the norm now, but it’s not the only sort of review there is. I’m old enough to remember processes that include in person reviews that were more like a dissertation defense or conference presentation.<p>The literature around this that shows that code review is a useful quality practice (in fact one of the only useful quality practices) comes mostly from much more structured review processes than we see now.<p>My personal opinion is that before llms the GitHub style pr review was for making us feel better about our processes (or governance checkbox checking) and the age of llms will sweep them away as the cost/benefit is so much worse now.
Not all bugs are buffer overflows, many are just the code not doing what it claimed at a high level
> The primary purpose of code review is to find code that will be _hard to maintain_.<p>This makes me wonder if we all have a different primary purpose in mind when it comes to code reviews because that wouldn't be my number one. Talk within your teams would be my advice. Especially now with AI enabling more rapid changes.
What if I told you that understanding what it is doing and finding bugs is actually the same problem?
Personally, I would tell you that whatever understanding you gain may still have bugs. Unless your understanding is as complete as a formal treatment of the code, then there may still be bugs in the code due to shared misunderstandings between author and reviewer. The biggest one is both having an incomplete understanding of what a library function does.<p>So while there may be some overlap, particularly if each person has full understanding of the code's dependencies, in the general case, understanding code and finding bugs are quite different aims.
Found plenty of bugs by reading/doing code review.
As other's already pointed out, the author argues about the primary intent of code review. Of course you find bugs while doing it, and that's a nice side effect, but doing code reviews to assert correctness is maybe suboptimal QA. At least that is my take ...
100% this. This is why it's so hard to trust AI on code reviews, at least for now.
Well, the code review should also be reviewing the provided test code or test plan or whatever that will prove it does not have bugs.<p>You're not reviewing the code to confirm that the code is bug free... you're reviewing the additional code that confirms that the feature-code is bug free.<p>Any process that has a step of "we'll get to that later" is a failure. That includes testing. Until there is some provided content that will be able to provide evidence that that code is safe to merge, it's not done.<p>But yeah, I need to be able to understand what every line does.
Exactly. The procedure is to read the description of the change to understand its motivation, goals, and overall design. Then you read the tests, checking whether they are compatible with and cover all aspects of what was described. Then you can read the code under test but at that point you enjoy the assumption that it at least passed those tests.
Whatever purpose code review served pre-2002, post-2002 it serves as corporate audit coverage. A common (mis?) interpretation of SOX and SOC2 is essentially that the company must have a two-person sign-off on any system change that could damage the company or its reputation.
One of my favorite little things to notice is when everybody thinks they know what something is, and they all agree about it, but they in fact don't agree. In this case we have the statement "Code review is a good idea". What right-minded software engineer could possibly disagree with that?<p>But then notice 1. the number of people jumping up to say "No, you don't understand the point of code review" and 2. how what follows "The point is..." varies between so many different people. I can't quite say it's a unique take per person, as I've seen before, there are some common threads, but they are also not all the same answer by any means either.<p>In this case, there isn't a "the" point of code review to discuss. It turns out that while we all may have thought we were doing it for the same reasons, we aren't. This is real. We don't have the same goals, we don't have the same methodology, and thus, the value we get from it may be different. And in fact it is perfectly reasonable to discuss the multiple cost/benefits ratios that differ across the various definitions, because the simplification "it's good, end of story" is destroying important distinctions.<p>In this situation, it is helpful to frame this as a matter of the costs and benefits of the various options available. Forget the statement "code review is good"; it is fallacious to start with that statement as an axiom and then argue about whether or not your definition of "code review" is or is not the "correct" definition so that your definition gets the "good" attribute applied to it. Consider the options directly.<p>(I have to admit I've used this effect in anger... in meetings where I can tell that everybody thinks they know what some project is but I can tell they all have a different definition of it in mind, but I also know it's not going to happen anyhow, I don't chase down the differences. Sometimes you can use this to your advantage to cut short what would otherwise be a quite interminable, yet ultimately pointless, meeting.)
I think you're missing the point of code review.
By the time when the PR is ready to merge, discussions around the architecture and how the code should be structured should already be part of the tech design of a given feature. So the discussion around whether a A feature is built and planned in a maintainable way, should be way before a PR is filed.
A PR review is making sure that you verify against the already agreed-upon structure, making sure everything matches the plan, and also find bugs and stuff that was missed, according to the plan.
Uh.. why not both?
Because the reviewer is not magical. If there was something in the code the author couldn't see, the reviewer probably won't see it either.<p>The way to confirm that code does not have bugs is testing. So the reviewer is not looking at the code saying "this will work", they're looking at the code saying "I understand how this works, it makes sense."<p>Evidence that the code is <i>safe</i> is something that <i>also</i> should be provided in the PR, but it is <i>not</i> the main code. It is ideally test automation that is just as understandable as the feature code, but failing that ad-hoc test evidence or a specific step-by-step plan with evidence of execution is good too.
The primary purpose of code review is to maintain existing hierarchy by preventing junior SWEs from getting promoted by committing code that is smarter than what the senior architect can understand.
> many people misunderstand the purpose of code review<p>Oooh, I bet including the author? Yeah, right there, he fails to make any qualifications for his statement, making it factually incorrect.<p>There are plenty of reasons to do code review. If you force me to, I'll define it as information transfer. The point is to have a conversation about the code. To expand both people's understanding about the codebase. Everything on top of that is extra. I've found real and significant bugs doing code review. In large part because I understand the codebase better than the author. That's finding and preventing bugs.<p>I also read PRs looking for malicious code trying to sneak in, you never know, the person I've called my best friend, someone with opsec better than mine, may suddenly have turned evil and this is the PR where they're finally sneaking in the back door.... that's never happened yet, but fingers crossed!<p>> As everyone should know by now, it is not in general possible to find bugs by examining the code.<p>... I'd love to know what the author really meant to write here, because it certainly wasn't this.
He's a mathematician, so what he means by "in general", is "in every possible case", so what he means is, "not all bugs may be found by code review."