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.
Every team should follow a plan, fine on a side project, but if you work in a large codebase with a bunch of devs, you need to have some sort of workflow to avoid stepping on each other's toes.
bug fixes are supposed to be small, contained, if they're rearchitecting the codebase, then they're not _bugs_, but tech improvements, and need to be addressed differently and I agree that this should be flagged in the PR.
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 ...
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.
If the code is so smart that it's not easily understandable, it's not easily fixable. My transition from junior to senior was accompanied by the realization that simpler is nearly always better.
I don't know enough to speak about that particular domain, but if the junior is writing something the senior can't understand, that's always going to be a problem. That code becomes the team's responsibility, and that code needs to be able to be maintained by the team, not only by the junior with something to prove.
Who is getting called at 2 AM when something breaks? Not the junior.
this gives vibes of "oh, you like rock music? name every rock band ever".
there is an implicit "relatively" before "simple". if there is two implementations of "ai inference code that has high performance in big batches", and one is more simple than the other with ~all else the same, the simple one is better.
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".
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.
The other stuff (minor bugs, overly verbose code) can easily be fixed. Paradigm rot cannot.
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.
Maintainability is a major factor in that, of course.
Yep, without a decent team culture this is what LLMs force, the slop deluge is just overwhelming without leadership asserting "no, stop"
Ultimately you just let bugs through because the alternative is spend an inordinate amount of time communicating with someones claude through PR comments about what the shape should be.
Career was fun while it lasted. I suppose its a blessing a to get to do a job that you enjoyed for as many years as I did.
Disapprove and ask for a call where the author must verbally explain the changes to receive approval? This seems like a solvable problem, and one that already existed in repos with many contributors of varying skill (open source, bigco with lots of interns). Letting bugs through is an even bigger time sink.
If you work at a small shop that may work. Everyone is 100xer now thanks to AI. No one has time to actually go through all this nonsense, not even the people who "wrote" it. Approve and pray you're not the one on call is the only viable strategy for some.
It comes down to culture on the team, if you think your leadership will allow that/you won't get a bad perf review for doing it go for it. That is sadly not a lot of peoples reality.
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.
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."
Evidence that the code is safe is something that also should be provided in the PR, but it is not 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.
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.
He's a mathematician, so what he means by "in general", is "in every possible case", or "without exception", so what he means is, "not all bugs will be found by code review."
But a) that's not what his words mean in English and b) that's irrelevant.
"in general" quantifies over all occasions, not over all bugs on one occasion.
To quote my own response:
===
Apparently the mathematician author doesn't understand the meaning of his own natural language quantifiers. “it is not in general possible to find bugs by examining the code” means “it is not in general possible to find ANY bugs by examining the code”, not “it is not in general possible to find ALL bugs by examining the code”.
And the first interpretation is relevant but wrong, whereas the second interpretation is true but irrelevant.
P.S. It seems that the author meant to say “it is not in general possible to find a given bug by examining the code”, i.e., "not (for all bugs B it is possible to find B)", which again is true but not relevant.
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.
I would add that (related to your "maintainability" point) ensuring the code is as simple as possible, and thus much more likely to be "debuggable by review", is a goal of review. Even that won't prevent bugs in the absolute sense, as you rightly say, but it boosts your probabilities.
I guess that makes sense. Based on my math lectures during college, mathematicians can often be terrible at communication to other humans, so that would explain why they think what they said is different from how pretty much everyone else reads it.
Apparently the mathematician author doesn't understand the meaning of his own natural language quantifiers. “it is not in general possible to find bugs by examining the code” means “it is not in general possible to find ANY bugs by examining the code”, not “it is not in general possible to find ALL bugs by examining the code”.
And the first interpretation is relevant but wrong, whereas the second interpretation is true but irrelevant.
P.S. It seems that the author meant to say “it is not in general possible to find a given bug by examining the code”, i.e., "not (for all bugs B it is possible to find B)", which again is true but not relevant.
Thank you, I thought I was getting crazy! The self-proclaimed amateur mathematician[0] who quotes his own writing in 3rd person has some stuff to work on I guess.
"in general" in mathematics means "in all cases, without exception", rather different than the normal usage where it means "usually, but not always".
If a mathematician using the mathematical sense of the word general says "it is not in general possible to tell if a program will halt by inspecting it", they're talking about the halting problem, even if you've looked at lots of programs where you can tell if they'll halt or not, and your experience might be correctly described using the normal usage of the word with "in general I can tell if a program will halt or not".
I'm a mathematician AND I understand English. Your statement about the halting problem is not analogous to the one in question and your comment completely ignores my analysis, where I did in fact take "in general" to be universal. Again, what they apparently meant was “it is not in general possible to find a given bug by examining the code”, which is analogous to the halting problem statement, but is a completely irrelevant strawman since no one thinks or claims that code review can find all bugs. (Similarly, it is not in general possible to determine whether a given program halts, although it is always possible to determine that some programs halt ... but this is disanalogous because the halting problem is concerned with the "all" case whereas code review is concerned with the "any" case.)
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.
This flippant note is mostly more self-justification for being a lazy code reviewer.
Agreed. There's a whole checklist of what to look out for:
- Does it functionally achieve what it sets out to (as per tacker issue or PR description)?
- Does it have extraneous code? Leftover debug prints, private API keys etc...
- Does it have any obvious defects? Memory leaks, un-handled edge cases, security flaws, obsolete API calls, etc...
- Could it be more understandable? Add/remove abstractions, better variable/method names, more/less functional etc...
- Is the style consistent with the codebase and/or style guidelines?
- Are there obvious performance improvements? Hashset instead of list, lazy evaluations, etc...
- Is it sufficiently well tested?
I'm not even sure I agree that if I can't understand the code then it shouldn't go through. Some code is just really hard to understand. The aim is to make it as easy as possible to understand, while being functionally correct.
Indeed, we worked hard as an industry to move beyond "blame the author" to "blame the process/team".
Unfortunately this article is just bait, may as well say "People seem to think dinner is about eating food, but it's not about eating at all, actually it's about connecting with family and friends!". It's a specific type of poorly constructed reductionist argument that plays well on HN.
Totally agree. With the speed at which code can be written and deployed today due to AI, the emphasis should shift onto the review. Does the code actually run properly, are all of our assumptions correct, and are there any unintended side effects?
I've found the review and debugging process to be much more time consuming than writing/producing code, and just "praying it works" never ends well.
The problem with this is that so many diffs created by AI are gigantic. Thousands and thousands of lines of code, per PR. Of course this is fine if AI is creating, reviewing AND merging, but it's an exercise in futiliy for human reviewers, as there is no way for them to catch up.
Companies that allow this will quickly lose control over code quality. The company should have PR size guidelines, and/or developers need to feel confident that they can say "please split into smaller PRs"
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?
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.
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.
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.
(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.)
> What right-minded software engineer could possibly disagree with that?
All of them? No matter what "code review" ultimately means it will have tradeoffs making its use a bad idea at least sometimes. There is no engineer who could agree with that statement. Suffice to say that the software industry attracts a lot of non-engineers, however.
What I find to be maybe the single most important part of code review is knowledge transfer.
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 often.
Beyond that, it gives a forum to ask questions about how things work to further build understanding.
Another important 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 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.
It's a good practice. Worth mentioning also: the same can be done with ordinary git log, assuming everyone is using git well. A proper git log of yesterday's work can be like your work newspaper with coffee.
Becoming very comfortable with "rebase --interactive" and other cmds for editing your (local!) history before merging helps a lot. Once you are, it only adds 5m or so of extra work to most PRs. And while acquiring this knowledge used to be difficult, LLMs make it very easy these days.
I would also recommend an editor designed for rebases. I use the nodejs rebase-editor TUI (though it looks like the old non-vibecoded releases have been removed from github, so unclear on the current availability of this one) which makes it easier to organize
You can setup most PR systems to squash on merge using the first commit's message, then enforce that the top commit message is prefixed with a ticket ID. This practice has many benefits: readable git log, easier git bisect for tracking down regressions, it becomes easy to find all commits associated with a block of work, more useful git blame.
I strongly recommend against auto-squashing. It creates large commits without the semantic intent of the author. In a large PR, you want clean, small, semantic commits, which makes it much easier to review and understand.
> 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
If code reviews are important, where does testing sit? Presumably if the coworker had not been part of the code review something would have stopped the breaking change making it's way to prod?
> If code reviews are important, where does testing sit?
Testing is for general boundaries. If you have a formalized specs, investing in an harness and writing a lot of tests is worth it.
But more likely in a corporate system, fully documenting everything is a huge hurdle by itself. So your best bet is to gather everyone that is related to a change and let them evaluate its impact.
Before the PR is cut, before the PR is sent to review (so that time isn't wasted on low-valuw bugs and problems --- engineers are expensive!), pre-merge and pre-release.
> 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.
How large is your team? Because I don't think that would scale beyond maybe five engineers
I'm a huge proponent of automated testing, because that catches things like "this whole system I depend on is gone" even if the guy who depends on it isn't in the room
I'm also a huge proponent of shared ownership of ... everything, really. It's natural for people to kind of own different pieces of a codebase, especially if it's a component they created, but that leads to silos and low bus counts. There shouldn't be one guy who owns one system that depends on one other component
> 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.
For non trivial or chore updates a second pair of eyes is always a good idea. But it’s not possible to scale out “everybody reads everything” to a large N. The problem is that nobody could keep up with that ad the reader when there are some huge number of things to read. That’s why we delegate, create docs, and have overview sessions.
> Our entire small team thumbs up a PR before it's merged unless there's a big rush on it
This has been tried by a couple of my past managers.
This feels great with a small team on a slow moving codebase. If you try to force it on a larger team or expect the codebase to move quickly then it turns into a performative game of skimming the code (if that) to click the thumbs up button so you can get back to your work.
The end game was a situation where nobody was really reviewing code because everyone had their own work to do and they didn’t want to be the one person blocking important PRs, so everyone was clicking thumbs up.
Based on your description, I don't think the process is working.
You describe a shared database, micro services you aren't aware of using the same database, and a desire for everyone to have a rough idea of the state of the codebase. And things breaking unexpectedly being caught by code review.
You have a bigger problem here of a system you can't reason about very well. The code review will help here, but I think you have bigger discoverability problems based on what I'm reading.
As others have said, what is needed is automated testing that would catch this sort of problem automatically without needing a human in the loop saying "Oh, wait...". We still want/need humans in the loop, but they should not be the only safety net.
This would be caught upstream in my org. If something was removed in a PR which was not documented and discussed with the entire team ahead of time, that would not be good for the submitter.
> The primary purpose of code review is to find code that will be _hard to maintain_.
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.
It’s probably important to define what sort of code review you are talking about when making broad claims about it.
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.
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.
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.
On one of my first jobs, I had printed off change packages which had to be reviewed and signed. There was even a person owning the final copies in filing cabinets. This was more like traditional engineering and everyone had to think of software as more permanent.
> 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.
Synchronous review is still possible today! One of my earliest managers taught me that if a "standard" code review goes back and forth more than once, it's almost always better to just hash it out in person (or on a Zoom call, when at least one person is remote) and then go back and post a comment summarizing what consensus was reached. To use a contorted technical analogy, asynchronous text communication can be lossier in terms of what information it's able to successfully encode than verbal, and the throughput is lower, so sometimes it's worth it to pay the synchronization overhead when you need to exchange a lot of information.
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.
This is why the solutions for high-trust environments (small teams) and low-trust environments (big companies, open source projects) will be different.
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.
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.
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.
If, by your definition of understanding, you can understand something without knowing how it works, then what would be the point in understanding it?
Your argument underscores the fact that understanding exists on a spectrum and that deep understanding can be difficult, or even impossible to achieve. Even formal verification seeks to verify certain properties of a system, and doesn't represent anything close to comprehensive understanding, and the process of formal verification is also guided by a fallible human. In a lot of ways, formal verification is just pushing up to review of a different "programming language."
So, good review is really hard, but ignoring the difficulty doesn't make it go away.
You’re confusing the difference between understanding and comprehending. The former is partial but useful within a context, the latter is total and is the result of mastery. One may understand an abstracted interface and how it works from the perspective of what it does in relation to solving a domain problem, but that doesn’t require them to know how it works at the level of any function/procedure’s mechanism.
While it may be true that formal verification doesn’t yield comprehensive understanding of code in certain forms, in a way it does by ensuring the code is only able to perform up to isomorphism with what the domain requires. Further understanding of how it accomplishes that is meaningless due to abstraction. But there are a number of techniques of formal methods that do require comprehensive understanding to be taken as valid, such as the structured proofs Dijkstra was famous for promoting. Also the classic example of Temporal Logic specs don’t compose, so the specifier must design the spec from scratch basically every time. Even using Hoare logic would require one to verify every line and all functions that are called for the purpose of solving a particular problem because of the difficulty of formally describing a function’s behavior in total.
So while I understand where you’re coming from, I do think that for any meaningful understanding regarding code, formal verification will provide it to the verifier. That is, as much as may reasonably be understood from the code is learned during any formal verification process.
This is largely my take as well. When I review code, I am checking for correctness. If I find something is not correct, that's a bug (or a bug waiting to happen). If I can't understand whether or not something is correct, that's a problem. If I don't know what the correct behavior should be, that's a problem.
Though I do think there is value in the original post. Re-framing is a powerful creative tool when you hit a mental dead end. And the responses let people share the other benefits that change management can bring.
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.
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.
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.
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.
This post is inverted for high assurance domains. For example, DO-178C requires checks for compliance to requirements, coding standards, traceability, accuracy and verifiability.
I work with someone who tends to rejects PR suggestions. I also work with someone else who accepts suggestions.
I think that the for the person who accepts suggestions, it's made me wonder if they accept them in part to share ownership with me. I feel like we both maintain and understand the code, and are on the same page.
For the person who rejects PR suggestions, it makes me less inclined to participate in those PRs. Why spend the time doing a thorough review if it's going to get rejected anyways.
> For the person who rejects PR suggestions, it makes me less inclined to participate in those PRs. Why spend the time doing a thorough review if it's going to get rejected anyways.
This is why you leave blocking suggestions and force the conversation if you think it is important enough.
Our team tends to prefix all our comments with one of
* thought: Maybe foo'ing is more common in the future - we can refactor if that happens.
* change: This is a leaky abstraction, would prefer to see this modeled like bar instead.
* nit: Naming seems a little unintuitive, consider "Baz", "Boo" maybe?
* fix: This unit test is validating the wrong field.
* chat: This is a big decision and would dictate how solutions of this category look like going forward. Let's bring this to the team first.
----
With the idea that some of those prefixes are stopping the PR until they are changed, and some are just a "take it or leave it" type comments. It makes it unambiguous to the opener that you consider these X things as "We've gotta get on the same page" and these Y things as "Stated preferences" or "just an observation".
word of warning - don't feel bad if you leave a nit, the other person disagrees and ignores it. If you felt strongly about it, it shouldn't have been a nit.
No, the real reason for the code review is to protect the moat of senior engineers/leaders that would nitpick on minute details of code while ignoring the big picture to make sure they can gatekeep any promotions and their competition.
Sure, but that doesn't mean it works that way everywhere or is inevitable.
Where I work, a senior would be reprimanded for such behavior. A key metric to their success is how well they can transfer knowledge and teach others, and their peers' feedback is used to determine this.
The senior that "punches down" does not go far in this case.
192 comments
[ 1.8 ms ] story [ 69.8 ms ] threadAlso such approach doesnt work with bug fixes / regressions
bug fixes are supposed to be small, contained, if they're rearchitecting the codebase, then they're not _bugs_, but tech improvements, and need to be addressed differently and I agree that this should be flagged in the PR.
a PR review is the final defence line before a QA
For bug fixes usually there are no fancy docs because until fix is found then it isnt known how to fix it (usually)
And after fix is found, there may be improvements applied to it
Who is getting called at 2 AM when something breaks? Not the junior.
there is an implicit "relatively" before "simple". if there is two implementations of "ai inference code that has high performance in big batches", and one is more simple than the other with ~all else the same, the simple one is better.
Bad programmers make the simplest things really complicated.
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.
The other stuff (minor bugs, overly verbose code) can easily be fixed. Paradigm rot cannot.
Maintainability is a major factor in that, of course.
Our team started using AI, so I switched to a simple method: no comments, and a binary "is this batshit crazy or passable" approval decision rule.
Saving myself time and sanity.
Ultimately you just let bugs through because the alternative is spend an inordinate amount of time communicating with someones claude through PR comments about what the shape should be.
Career was fun while it lasted. I suppose its a blessing a to get to do a job that you enjoyed for as many years as I did.
I'll be really interested to follow what comes out of the Bun team.
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."
Evidence that the code is safe is something that also should be provided in the PR, but it is not 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.
"in general" quantifies over all occasions, not over all bugs on one occasion.
To quote my own response:
===
Apparently the mathematician author doesn't understand the meaning of his own natural language quantifiers. “it is not in general possible to find bugs by examining the code” means “it is not in general possible to find ANY bugs by examining the code”, not “it is not in general possible to find ALL bugs by examining the code”.
And the first interpretation is relevant but wrong, whereas the second interpretation is true but irrelevant.
P.S. It seems that the author meant to say “it is not in general possible to find a given bug by examining the code”, i.e., "not (for all bugs B it is possible to find B)", which again is true but not relevant.
===
I would add that (related to your "maintainability" point) ensuring the code is as simple as possible, and thus much more likely to be "debuggable by review", is a goal of review. Even that won't prevent bugs in the absolute sense, as you rightly say, but it boosts your probabilities.
And the first interpretation is relevant but wrong, whereas the second interpretation is true but irrelevant.
P.S. It seems that the author meant to say “it is not in general possible to find a given bug by examining the code”, i.e., "not (for all bugs B it is possible to find B)", which again is true but not relevant.
[0]: https://blog.plover.com/meta/about-me.html
If a mathematician using the mathematical sense of the word general says "it is not in general possible to tell if a program will halt by inspecting it", they're talking about the halting problem, even if you've looked at lots of programs where you can tell if they'll halt or not, and your experience might be correctly described using the normal usage of the word with "in general I can tell if a program will halt or not".
I won't respond further.
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.
This flippant note is mostly more self-justification for being a lazy code reviewer.
- Does it functionally achieve what it sets out to (as per tacker issue or PR description)?
- Does it have extraneous code? Leftover debug prints, private API keys etc...
- Does it have any obvious defects? Memory leaks, un-handled edge cases, security flaws, obsolete API calls, etc...
- Could it be more understandable? Add/remove abstractions, better variable/method names, more/less functional etc...
- Is the style consistent with the codebase and/or style guidelines?
- Are there obvious performance improvements? Hashset instead of list, lazy evaluations, etc...
- Is it sufficiently well tested?
I'm not even sure I agree that if I can't understand the code then it shouldn't go through. Some code is just really hard to understand. The aim is to make it as easy as possible to understand, while being functionally correct.
Unfortunately this article is just bait, may as well say "People seem to think dinner is about eating food, but it's not about eating at all, actually it's about connecting with family and friends!". It's a specific type of poorly constructed reductionist argument that plays well on HN.
I've found the review and debugging process to be much more time consuming than writing/producing code, and just "praying it works" never ends well.
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.
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.
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.
(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.)
All of them? No matter what "code review" ultimately means it will have tradeoffs making its use a bad idea at least sometimes. There is no engineer who could agree with that statement. Suffice to say that the software industry attracts a lot of non-engineers, however.
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 often.
Beyond that, it gives a forum to ask questions about how things work to further build understanding.
Another important 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 had access to this table. The institutional knowledge check here though prevented a larger issue and potential data cleanup situation.
If code reviews are important, where does testing sit? Presumably if the coworker had not been part of the code review something would have stopped the breaking change making it's way to prod?
or a prod outage causes the knowledge to be experienced.
At least, that's what people do by default.
Testing is for general boundaries. If you have a formalized specs, investing in an harness and writing a lot of tests is worth it.
But more likely in a corporate system, fully documenting everything is a huge hurdle by itself. So your best bet is to gather everyone that is related to a change and let them evaluate its impact.
Everything else is just fantasy.
Looks like you had both.
How large is your team? Because I don't think that would scale beyond maybe five engineers
I'm a huge proponent of automated testing, because that catches things like "this whole system I depend on is gone" even if the guy who depends on it isn't in the room
I'm also a huge proponent of shared ownership of ... everything, really. It's natural for people to kind of own different pieces of a codebase, especially if it's a component they created, but that leads to silos and low bus counts. There shouldn't be one guy who owns one system that depends on one other component
For non trivial or chore updates a second pair of eyes is always a good idea. But it’s not possible to scale out “everybody reads everything” to a large N. The problem is that nobody could keep up with that ad the reader when there are some huge number of things to read. That’s why we delegate, create docs, and have overview sessions.
This has been tried by a couple of my past managers.
This feels great with a small team on a slow moving codebase. If you try to force it on a larger team or expect the codebase to move quickly then it turns into a performative game of skimming the code (if that) to click the thumbs up button so you can get back to your work.
The end game was a situation where nobody was really reviewing code because everyone had their own work to do and they didn’t want to be the one person blocking important PRs, so everyone was clicking thumbs up.
Then again, if you're dealing with an un-maintainable minefield of a code base then "fire the whole team, I dare you" might be what you're after.
You describe a shared database, micro services you aren't aware of using the same database, and a desire for everyone to have a rough idea of the state of the codebase. And things breaking unexpectedly being caught by code review.
You have a bigger problem here of a system you can't reason about very well. The code review will help here, but I think you have bigger discoverability problems based on what I'm reading.
As others have said, what is needed is automated testing that would catch this sort of problem automatically without needing a human in the loop saying "Oh, wait...". We still want/need humans in the loop, but they should not be the only safety net.
I started ignoring all PRs from our large team because we had a similar policy. My teammates can handle, they don't need me to check on each PR.
This would be caught upstream in my org. If something was removed in a PR which was not documented and discussed with the entire team ahead of time, that would not be good for the submitter.
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.
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.
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.
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.
Synchronous review is still possible today! One of my earliest managers taught me that if a "standard" code review goes back and forth more than once, it's almost always better to just hash it out in person (or on a Zoom call, when at least one person is remote) and then go back and post a comment summarizing what consensus was reached. To use a contorted technical analogy, asynchronous text communication can be lossier in terms of what information it's able to successfully encode than verbal, and the throughput is lower, so sometimes it's worth it to pay the synchronization overhead when you need to exchange a lot of information.
This is why the solutions for high-trust environments (small teams) and low-trust environments (big companies, open source projects) will be different.
https://crawshaw.io/blog/agent-principal-agent
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.
Your argument underscores the fact that understanding exists on a spectrum and that deep understanding can be difficult, or even impossible to achieve. Even formal verification seeks to verify certain properties of a system, and doesn't represent anything close to comprehensive understanding, and the process of formal verification is also guided by a fallible human. In a lot of ways, formal verification is just pushing up to review of a different "programming language."
So, good review is really hard, but ignoring the difficulty doesn't make it go away.
While it may be true that formal verification doesn’t yield comprehensive understanding of code in certain forms, in a way it does by ensuring the code is only able to perform up to isomorphism with what the domain requires. Further understanding of how it accomplishes that is meaningless due to abstraction. But there are a number of techniques of formal methods that do require comprehensive understanding to be taken as valid, such as the structured proofs Dijkstra was famous for promoting. Also the classic example of Temporal Logic specs don’t compose, so the specifier must design the spec from scratch basically every time. Even using Hoare logic would require one to verify every line and all functions that are called for the purpose of solving a particular problem because of the difficulty of formally describing a function’s behavior in total.
So while I understand where you’re coming from, I do think that for any meaningful understanding regarding code, formal verification will provide it to the verifier. That is, as much as may reasonably be understood from the code is learned during any formal verification process.
Though I do think there is value in the original post. Re-framing is a powerful creative tool when you hit a mental dead end. And the responses let people share the other benefits that change management can bring.
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.
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.
But yeah, I need to be able to understand what every line does.
I think that the for the person who accepts suggestions, it's made me wonder if they accept them in part to share ownership with me. I feel like we both maintain and understand the code, and are on the same page.
For the person who rejects PR suggestions, it makes me less inclined to participate in those PRs. Why spend the time doing a thorough review if it's going to get rejected anyways.
This is why you leave blocking suggestions and force the conversation if you think it is important enough.
* thought: Maybe foo'ing is more common in the future - we can refactor if that happens.
* change: This is a leaky abstraction, would prefer to see this modeled like bar instead.
* nit: Naming seems a little unintuitive, consider "Baz", "Boo" maybe?
* fix: This unit test is validating the wrong field.
* chat: This is a big decision and would dictate how solutions of this category look like going forward. Let's bring this to the team first.
----
With the idea that some of those prefixes are stopping the PR until they are changed, and some are just a "take it or leave it" type comments. It makes it unambiguous to the opener that you consider these X things as "We've gotta get on the same page" and these Y things as "Stated preferences" or "just an observation".
word of warning - don't feel bad if you leave a nit, the other person disagrees and ignores it. If you felt strongly about it, it shouldn't have been a nit.
Where I work, a senior would be reprimanded for such behavior. A key metric to their success is how well they can transfer knowledge and teach others, and their peers' feedback is used to determine this.
The senior that "punches down" does not go far in this case.
Well kinda - code review needs to identify any missing tests? And without the tests more likely a bug could exist.