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.
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
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:
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 I've worked.
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.
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 existed let alone had access to this table. The institutional knowledge check here though prevented a larger issue and potential data cleanup situation.
> 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.
> 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
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.
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.
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
> 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 its 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.
> 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.
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.
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.
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.
> it is not in general possible to find bugs by examining the code.
Oh hell yes it is, at every level of abstraction even. We call those things code smell... A file descriptor that hasn't been closed, a coroutine that hasn't been awaited, a big try/catch block that just falls back to some value without logging the error, wrong type castings, etc.
As a general rule: Neither type checker, nor compiler, nor runtime should ever be steps that merely want to be satified - work with these steps and treat them as the valuable tools they are, and never work against them.
Yeah, I have no idea what they're talking about with that one. I've caught bugs when reviewing code without needing to run it before, and I've had the same happen to me in reverse, and I've seen it happen between others on reviews I was observing. I guess they could find some way to define "in general" so that this is technically true, but at that point it's not particularly meaningful.
True - the biggest thing I want to catch in an MR is "will this change lead us onto a path that 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.
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 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.
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.
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.
Code review isn’t a singular thing. There are many reasons for code review, like knowledge sharing, liability laundering, code quality, regulatory compliance, etc. As usual, what purpose it serves depend on your use case.
Thank you! I find it somewhat ignorant to say, most people do not understand the reason behind a peer review, while obviously being missinformed. To even believe that it only serves a single purpose somewhat tells me, that the author is missing experiences with other teams / people.
> To even believe that it only serves a single purpose
There are a lot of people who incepted the word "single" into this post in order to have something worthless to argue about. Even with "primary" as the second word of the headline.
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 seens to misunderstand the purpose of code review. The purpose is, literally, review the code. Review means basically to think/talk about something again in order to make or not changes on it. When you review something (including code), you are basically asking for yourself: "Should it be changed or it's okay to stay like this?"
In order words, the purpose of code review is to or not ask for changes on the code.
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.
The primary purpose of code review is whatever your organization decides it wants it to be. The post makes a good enough point but undermines it with this dogma.
It seems the form that code review takes depends on the structure of the organization. In practice, some places use code review as a way to assert hierarchy and tear someone down, while in others it's a friendly, knowledge-sharing exchange. Code review varies depending on the organization's shape and the manager.
And I agree to some extent with what the OP's tweeter said.
these days, bugs can look perfectly fine on the surface, but when combined with the existing system, entirely new types of bugs emerge. This is an especially common pattern in the AI era: the added code itself isn't the problem, but it becomes a bug once it interacts with the existing codebase.
> The primary purpose of code review is to find code that will be _hard to maintain_.
In some organizations, maintainability may be the biggest risk being mitigated in a code review. But for me, that's selling code reviews short.
In my experience, code reviews are the single most important quality control process in the entire development life cycle. Engineers often don't have a lot of influence over the quality of requirements. Engineers often don't have a lot of influence over the competence and thoroughness of the QA process (and it often doesn't exist at all). But engineers frequently have total control over code reviews.
If I can't depend on the rest of the organization for QC, code reviews are the first place I look to mitigate that risk. That means code reviews find bugs. That means code reviews identify code smells. That means code reviews pressure test requirements and whether the implementation matches the assignment. That means code reviews transfer knowledge and serve as a teacher for both the PR author and the reviewer. And so on.
Thorough and pedantic code reviews are challenging and tedious, at least at first, but the team adapts and both the code and the review process gets better.
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.
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.
> 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.
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 appreciate the perspective, but code reviews are subjective. The tooling and the language can be so good that it shifts to this kind of utopian state, where all bugs are caught and eliminated by guarantees in the language and tooling. Or they could be dismal to the point a human needs to check for mundane issues like bugs in the code.
Maintainability is incredibly important, but acting like bug-catching is just a "happy little accident" of code review is an over correction. A good code review should make sure the code is readable and that it actually does what it’s supposed to do without breaking the system. You need to do both.
There are many peripheral nice-to-haves you can get from a code review. Bugs, security, performance, correctness issues are all possible bonus findings you may get sometimes.
But there is only one _must_ in reviews: another person reading and understanding the code, possibly suggesting architectural improvements, or asking questions that should be answered by rephrasing the code for clarity, or by adding code comments. In other words: maintainability. That's the one thing that's not a bonus point, and is a constant for all code reviews.
> The primary purpose of code review is to find code that will be _hard to maintain_
Says who? I agree that it's a good purpose. But the main problem with code reviews is not having a in-house code review guidance. Doesn't need to be long. 1 page would do wonders. Then we're all on the same page.
I remember a time when the sole point of code review was to answer the question 'is this change going to break any other work currently in-progress?' That was it.
it was expected to be a short-duration fixed-scope activity with a clear Go-NoGo answer, and you would have gotten roasted for digressing into navel-gazing and distracted conversations about armchair philosophy. if the change worked as-advertised and didn't break your work, then it was a 'go', get back to work...
Well, we are dumb and we make stupid mistakes, and code review can find and fix them sometimes. I am very surprised that the practice of "review" is not more widespread. For example when I see medical doctors going YOLO with their patients, ppl driving excavators, etc.
> “Programs must be written for people to read, and only incidentally for machines to execute.”
people probably did not realise what it meant, but AI is bringing it to forefront. Huge amount of work we do is essentially to communicate decisions we want to take, are going to take or have taken. It is a cornerstone of our society when people are continuously exposed to the relevant decisions which are taken in order to build a shared understanding and move forward.
Programming was nothing but that. We did not have a good enough compiler till now and we definitely do not have a good enough language to describe what we need (mostly because thoughts and world in general are so much more complex than any language). And therefore, we used the same language for computer to execute our code and for us to read and understand it. But the reason we store our code in human readable language and not machine code is because we want to communicate the code to future self.
That's why Elon musks's statement about just directly getting a binary makes no sense, because the language used to specify that binary needs to be stored in some engineering records anyway, and then "that is the code".
Code review is also exactly the same, it is a signal which says, "I want to take this decision, are you okay with us taking this decision?" and everyone interested signs off.
Bugs finding are just people going, "I completely agree with the principle of the decision but this particular part of decision is anti-thetic to the principle, should we fix it?"
I've been at a lot of companies where code is held hostage by the reviewers, specifically leads or wannabe leads. Interfaces matter. Maintainability matters. Memory allocation certainly matters. Code reviews frequently go down the rabbit hole of how the reviewer would prefer to design it and a long back and forth until the developer has, "explained himself", like it's a Breaking Bad confrontation. I think it devolves into a status battle.
without knowing the people involved, or any further nuance/context -- i had a PhD supervisor like this. he was always looking for holes. always disliked the way i was proposing something. always looking to find the thing that's wrong. the way to deal with him was to have thorough answers to as many questions he might ask before he asks them. learning to predict his questions was the real training for my PhD.
it taught me to show my working, as if i were in math class. which meant fully writing things up before meeting with him.
> how the reviewer would prefer to design it
sounds like they're trying to find holes. dunno if you do this already, but if not, try to fill the holes beforehand with conclusive and extensive write up (show the working). maybe ask them before coding how they would design X and then dedicate a section of the write up to why X cannot or was not designed that way.
answer their questions before they have a chance to ask the questions.
"it is not in general possible to find bugs by examining the code"
That really depends on the quality of developers you are dealing with.
I certainly find myself assessing code quality, performance issues, shortcomings. Occasionally trivial bugs. Most importantly you review for taste, I'm tasting the code.
Maintainability is definitely important but is pretty subjective and is a subset of taste in general.
The other issue with code review, and I'm glad I've not worked with people like this anymore, for the person being reviewed: NOBODY IS ATTACKING YOU, nobody is saying your code is bad, the goal is to do a once-over for quality.
Another goal people often miss:
It's okay to ask "stupid questions" and I would argue as a Junior, ask away, even if no code changes happen, ASK. Kind of follows the spirit of the original post, which is, can you maintain this?
Not true. I’ve attacked people before in the sense I was disgusted with the short cuts they took and the things they tried to get away with in the code.
I’ve seen the malicious deletion of features in order to stay hidden and I’ve called it out.
That's not always an attack though, at least in a constructive critical review, maybe the approach could be different, but I'm also talking about when people are leaving comments that are genuine, professional, and tone is fine. In your case, yes you are attacking them I suppose, even though you don't really need to, you just need to rejec the PR and comment "hey, are you sure this is right?" and substantiate it. Sometimes people don't realize.
There more things in a pull request other that just reviews. It can run a build pipeline and give feedback, provision test instances to run quick tests.
It can also be used to add context and have a conversation around the why's before merging. And yeah while it's not the main purpose we've caught a lot of bugs too just from the statically reviewing code.
Agree and disagree. It seems hyperbolic to say code review isn't to find bugs; of course it is. That happens every week on my teams. But the main point is the knowledge transfer and readability of code.
> 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.
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.
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.
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 ...
Code reviews are still a critical step in most workflows. Though seems like everyone uses them for a different purpose: extra pair of eyes to meet a regulation/security, style police, and what this one says: maintainability.
And at the same time, code reviews are now a massive bottleneck in the development pipeline. Frankly, in a lot of teams they have been that for a long time. Though many would argue it was the only thing stopping absolute crap going into production.
But in a world where none of us writes code anymore, and I think we're there, even if the future is "just not evenly distributed" (Gibson), why should we have to do code reviews, the worst job of all?
Leaves me thinking that code reviews will land in the dustbin of history.
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.)
My employer has the weirdest code reviews I have ever participated in. I kid you all not, we all get around and it's like team-wide show and tell. We basically demo anything cool we learned, found, etc.. There is actually no real review of any code at all. At least, not in terms of quality or security.
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.
It's kinda funny that we all agree that code review is important but we cannot agree why exactly it is important. Imagine a bunch of ancient shamans arguing why exactly local volcano needs to be fed one virgin a year but all being in agreement that it does.
> As everyone should know by now, it is not in general possible to find bugs by examining the code.
Lost me here. I would agree that it’s not possible to find every bug by examining code, but in real code reviews bugs and errors are identified by reviewers all the time. Reviewers lend their past experience to the situation, identify some unnoticed interaction, think of an edge case that the author hadn’t considered, or some times just notice simple logic errors.
Code review is a fresh set of eyes. When we write code eventually parts of it get accepted in our minds as done or correct and we can start missing things that are obvious to a reviewer.
I’m not a fan of these blanket declarations that code review isn’t about reviewing code. I’ve read countless hot takes like this that code review is about some other thing (finding unmaintainable code, knowledge transfer, etc) that all miss the point that code review isn’t about one thing. These reductions and exclusions can be really misleading.
I would add to this other purposes of code review:
* to share knowledge among the team — if code has been reviewed then at least two people know about it
* to ensure the changes won’t cause problems with other systems or future plans — maybe we will be rolling out a new logging system soon and we don’t want to solve the problem in this specific way
* a chance for the reviewer and reviewee to learn something about the system or the code via the review discussion
* team coherence — code that’s well reviewed is a team effort. Working together on small things like code reviews helps teammates work better together on other tasks in the future
I don't know about many people, but the author for sure doesn't understand the primary purpose of code review.
If the primary purpose of code review is to assess maintainability, there is no need for review, that can be done by automated tooling (formatting, bad naming, cyclomatic complexity etc.)
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.
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.
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.
> many people misunderstand the purpose of code review
Oooh, I bet including the author? Yeah, right there, he fails to make any qualifications for his statement, making it factually incorrect.
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.
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!
> As everyone should know by now, it is not in general possible to find bugs by examining the code.
... I'd love to know what the author really meant to write here, because it certainly wasn't this.
> 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.
this. was a lead in small businesses working with very inexperienced people in one team -- literally having to teach them git -- and another with outsourced devs who had one day a week for us and could barely remember cos they context switched onto other projects for the other 4 days a week etc.
my job was to always know the most about the systems, what we're trying to achieve with the systems and what is in the codebases. answer questions. send links to people. know which devs to ask for more detail etc. i never necessarily knew everything about frameworks, libraries, package updates, new tools etc., but i knew our system/devs/goals/products inside and out.
as i said to several people, your first PR submission is just your first public draft. i will find bugs. i will find things that need changing. it's not personal. it's just about making your PR better than it was. this is an opportunity to learn from me.
the author of the article seems to be considering code review from an idealised situation -- this never occurs in reality, most of us have to make do.
and that means changing focus of code review for what is needed, rather than what is ideal.
He's a mathematician, so what he means by "in general", is "in every possible case", or "without exception", so what I think he means is, "not all bugs will be found by code review." I agree it probably could have been made more clear.
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 entire team, not only by the junior with something to prove.
Who is getting called at 2 AM when something breaks? Not the junior.
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.
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.
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 I've worked.
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.
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 existed let alone had access to this table. The institutional knowledge check here though prevented a larger issue and potential data cleanup situation.
> 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.
This with a side helping of "when everyone is responsible nobody is"
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.
> 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
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.
this seems like a chain of good practices. though I find it hard to stay disciplined about keeping commits well scoped and well described
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.
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
this is a great way to train people to ignore PR emails/notifications
Maybe but those people are also having claude summarize their inbox and get nice little daily reports on things they might want to look into.
> 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 its way to prod?
> something would have stopped the breaking change making its way to prod?
or a prod outage causes the knowledge to be experienced.
I'd guess testing is done only for the software being deployed, not for that other microservice.
At least, that's what people do by default.
> 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.
> 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.
Decisions by small groups should be the default. Others only need to be involved if the risk/consequences of failure are high.
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.
Complexity is caused by Dependencies and Obscurities ~ John Ousterhout
Looks like you had both.
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.
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.
Such a luxury, I am envious!
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.
In other words, AI code is owned by nobody.
AI code is owned by owners and society is figuring out in real-time if code has some inherent value without humans that understand it.
I'll be really interested to follow what comes out of the Bun team.
It's not innacurate to say that AI code becomes legacy code the instant it's merged.
Tragedy of the commons
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.
This just makes reviewers and authors lazier.
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.
Unmaintainable is a bug. You just decided that you think code review is only meant to find one type of bug. Cool?
> it is not in general possible to find bugs by examining the code.
Oh hell yes it is, at every level of abstraction even. We call those things code smell... A file descriptor that hasn't been closed, a coroutine that hasn't been awaited, a big try/catch block that just falls back to some value without logging the error, wrong type castings, etc.
As a general rule: Neither type checker, nor compiler, nor runtime should ever be steps that merely want to be satified - work with these steps and treat them as the valuable tools they are, and never work against them.
Yeah, I have no idea what they're talking about with that one. I've caught bugs when reviewing code without needing to run it before, and I've had the same happen to me in reverse, and I've seen it happen between others on reviews I was observing. I guess they could find some way to define "in general" so that this is technically true, but at that point it's not particularly meaningful.
True - the biggest thing I want to catch in an MR is "will this change lead us onto a path that 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.
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 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.
User names match... are you the original author? Why commenting in the third person?
Because it was him... 10 months ago?
saghm still thought this was a weird explanation when he wrote this comment
it's still weird. if i'm talking about something i wrote 10 months ago i'd say something like, "when i originally wrote this i was trying to say ...".
not "when the author wrote this, he was trying to say"
Ofc you're correct in that sense.
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.
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.
Code review isn’t a singular thing. There are many reasons for code review, like knowledge sharing, liability laundering, code quality, regulatory compliance, etc. As usual, what purpose it serves depend on your use case.
Thank you! I find it somewhat ignorant to say, most people do not understand the reason behind a peer review, while obviously being missinformed. To even believe that it only serves a single purpose somewhat tells me, that the author is missing experiences with other teams / people.
> To even believe that it only serves a single purpose
There are a lot of people who incepted the word "single" into this post in order to have something worthless to argue about. Even with "primary" as the second word of the headline.
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 seens to misunderstand the purpose of code review. The purpose is, literally, review the code. Review means basically to think/talk about something again in order to make or not changes on it. When you review something (including code), you are basically asking for yourself: "Should it be changed or it's okay to stay like this?"
In order words, the purpose of code review is to or not ask for changes on the code.
> Review means basically to think/talk about something again
The rest is not part of the definition; critics don't review movies in order to change them.
But just accepting that at face value, making code more readable is a "change." I don't know what you're trying to say here.
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.
https://crawshaw.io/blog/agent-principal-agent
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.
The primary purpose of code review is whatever your organization decides it wants it to be. The post makes a good enough point but undermines it with this dogma.
It seems the form that code review takes depends on the structure of the organization. In practice, some places use code review as a way to assert hierarchy and tear someone down, while in others it's a friendly, knowledge-sharing exchange. Code review varies depending on the organization's shape and the manager.
And I agree to some extent with what the OP's tweeter said. these days, bugs can look perfectly fine on the surface, but when combined with the existing system, entirely new types of bugs emerge. This is an especially common pattern in the AI era: the added code itself isn't the problem, but it becomes a bug once it interacts with the existing codebase.
> The primary purpose of code review is to find code that will be _hard to maintain_.
In some organizations, maintainability may be the biggest risk being mitigated in a code review. But for me, that's selling code reviews short.
In my experience, code reviews are the single most important quality control process in the entire development life cycle. Engineers often don't have a lot of influence over the quality of requirements. Engineers often don't have a lot of influence over the competence and thoroughness of the QA process (and it often doesn't exist at all). But engineers frequently have total control over code reviews.
If I can't depend on the rest of the organization for QC, code reviews are the first place I look to mitigate that risk. That means code reviews find bugs. That means code reviews identify code smells. That means code reviews pressure test requirements and whether the implementation matches the assignment. That means code reviews transfer knowledge and serve as a teacher for both the PR author and the reviewer. And so on.
Thorough and pedantic code reviews are challenging and tedious, at least at first, but the team adapts and both the code and the review process gets better.
"The purpose of code review is not for the reviewer to find bugs"
Well kinda - code review needs to identify any missing tests? And without the tests more likely a bug could exist.
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.
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.
> 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.
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 appreciate the perspective, but code reviews are subjective. The tooling and the language can be so good that it shifts to this kind of utopian state, where all bugs are caught and eliminated by guarantees in the language and tooling. Or they could be dismal to the point a human needs to check for mundane issues like bugs in the code.
Maintainability is incredibly important, but acting like bug-catching is just a "happy little accident" of code review is an over correction. A good code review should make sure the code is readable and that it actually does what it’s supposed to do without breaking the system. You need to do both.
I think the author is right.
There are many peripheral nice-to-haves you can get from a code review. Bugs, security, performance, correctness issues are all possible bonus findings you may get sometimes.
But there is only one _must_ in reviews: another person reading and understanding the code, possibly suggesting architectural improvements, or asking questions that should be answered by rephrasing the code for clarity, or by adding code comments. In other words: maintainability. That's the one thing that's not a bonus point, and is a constant for all code reviews.
> The primary purpose of code review is to find code that will be _hard to maintain_
Says who? I agree that it's a good purpose. But the main problem with code reviews is not having a in-house code review guidance. Doesn't need to be long. 1 page would do wonders. Then we're all on the same page.
Look at the state of the comments on that thread.
"This is completely false! Code is review is to ..." proceeds to state an opinion.
Sometimes, some days, I just look forward to not having to deal with programmer hubris ever again.
I remember a time when the sole point of code review was to answer the question 'is this change going to break any other work currently in-progress?' That was it.
it was expected to be a short-duration fixed-scope activity with a clear Go-NoGo answer, and you would have gotten roasted for digressing into navel-gazing and distracted conversations about armchair philosophy. if the change worked as-advertised and didn't break your work, then it was a 'go', get back to work...
Well, we are dumb and we make stupid mistakes, and code review can find and fix them sometimes. I am very surprised that the practice of "review" is not more widespread. For example when I see medical doctors going YOLO with their patients, ppl driving excavators, etc.
I think when the the SICP authors said
> “Programs must be written for people to read, and only incidentally for machines to execute.”
people probably did not realise what it meant, but AI is bringing it to forefront. Huge amount of work we do is essentially to communicate decisions we want to take, are going to take or have taken. It is a cornerstone of our society when people are continuously exposed to the relevant decisions which are taken in order to build a shared understanding and move forward.
Programming was nothing but that. We did not have a good enough compiler till now and we definitely do not have a good enough language to describe what we need (mostly because thoughts and world in general are so much more complex than any language). And therefore, we used the same language for computer to execute our code and for us to read and understand it. But the reason we store our code in human readable language and not machine code is because we want to communicate the code to future self.
That's why Elon musks's statement about just directly getting a binary makes no sense, because the language used to specify that binary needs to be stored in some engineering records anyway, and then "that is the code".
Code review is also exactly the same, it is a signal which says, "I want to take this decision, are you okay with us taking this decision?" and everyone interested signs off.
Bugs finding are just people going, "I completely agree with the principle of the decision but this particular part of decision is anti-thetic to the principle, should we fix it?"
I've been at a lot of companies where code is held hostage by the reviewers, specifically leads or wannabe leads. Interfaces matter. Maintainability matters. Memory allocation certainly matters. Code reviews frequently go down the rabbit hole of how the reviewer would prefer to design it and a long back and forth until the developer has, "explained himself", like it's a Breaking Bad confrontation. I think it devolves into a status battle.
without knowing the people involved, or any further nuance/context -- i had a PhD supervisor like this. he was always looking for holes. always disliked the way i was proposing something. always looking to find the thing that's wrong. the way to deal with him was to have thorough answers to as many questions he might ask before he asks them. learning to predict his questions was the real training for my PhD.
it taught me to show my working, as if i were in math class. which meant fully writing things up before meeting with him.
> how the reviewer would prefer to design it
sounds like they're trying to find holes. dunno if you do this already, but if not, try to fill the holes beforehand with conclusive and extensive write up (show the working). maybe ask them before coding how they would design X and then dedicate a section of the write up to why X cannot or was not designed that way.
answer their questions before they have a chance to ask the questions.
"it is not in general possible to find bugs by examining the code"
That really depends on the quality of developers you are dealing with.
I certainly find myself assessing code quality, performance issues, shortcomings. Occasionally trivial bugs. Most importantly you review for taste, I'm tasting the code.
Maintainability is definitely important but is pretty subjective and is a subset of taste in general.
The other issue with code review, and I'm glad I've not worked with people like this anymore, for the person being reviewed: NOBODY IS ATTACKING YOU, nobody is saying your code is bad, the goal is to do a once-over for quality.
Another goal people often miss:
It's okay to ask "stupid questions" and I would argue as a Junior, ask away, even if no code changes happen, ASK. Kind of follows the spirit of the original post, which is, can you maintain this?
>NOBODY IS ATTACKING YOU
Somehow this captures a lot of the culture for me.
Places I work at weed out for this. If people cant take peer review they sour the workday for others.
You were not hired to write perfect code, relax.
Not true. I’ve attacked people before in the sense I was disgusted with the short cuts they took and the things they tried to get away with in the code.
I’ve seen the malicious deletion of features in order to stay hidden and I’ve called it out.
That's not always an attack though, at least in a constructive critical review, maybe the approach could be different, but I'm also talking about when people are leaving comments that are genuine, professional, and tone is fine. In your case, yes you are attacking them I suppose, even though you don't really need to, you just need to rejec the PR and comment "hey, are you sure this is right?" and substantiate it. Sometimes people don't realize.
There more things in a pull request other that just reviews. It can run a build pipeline and give feedback, provision test instances to run quick tests.
It can also be used to add context and have a conversation around the why's before merging. And yeah while it's not the main purpose we've caught a lot of bugs too just from the statically reviewing code.
Agree and disagree. It seems hyperbolic to say code review isn't to find bugs; of course it is. That happens every week on my teams. But the main point is the knowledge transfer and readability of code.
> 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.
In most projects I worked on it is plain burocracy to ensure the solution architect is happy with their hexagonal or clean architecture.
Really I can count up to five the amount of projects where good code review actually took place.
What if I told you that understanding what it is doing and finding bugs is actually the same problem?
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.
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.
100% this. This is why it's so hard to trust AI on code reviews, at least for now.
I make code review and PR approval optional for my teams. A solid process should emerge from respect and peer pressure alone.
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 ...
It's also important to help other members of the team to learn and understand.
Not all bugs are buffer overflows, many are just the code not doing what it claimed at a high level
Nowadays the main point of PR's is to find out what Claude has been coding for me.
lol, but also, nearly every comment in this thread seems so old school, as if the game hasn't completely changed.
PRs for knowledge transfer? I don't need it, I'll just ask an LLM a question if I have one.
PRs for enforcing good design? I dunno, if it's a terrible design, we'll just change it since change is so much less work now.
The purpose of code review is to talk about the purpose of code review ...
I don't know, but this new definition seems to be very AI friendly and matches the recent transformation of this blogger.
This is the big thing I'm struggling with:
Code reviews are still a critical step in most workflows. Though seems like everyone uses them for a different purpose: extra pair of eyes to meet a regulation/security, style police, and what this one says: maintainability.
And at the same time, code reviews are now a massive bottleneck in the development pipeline. Frankly, in a lot of teams they have been that for a long time. Though many would argue it was the only thing stopping absolute crap going into production.
But in a world where none of us writes code anymore, and I think we're there, even if the future is "just not evenly distributed" (Gibson), why should we have to do code reviews, the worst job of all?
Leaves me thinking that code reviews will land in the dustbin of history.
But for now I don't have a better solution.
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.)
My employer has the weirdest code reviews I have ever participated in. I kid you all not, we all get around and it's like team-wide show and tell. We basically demo anything cool we learned, found, etc.. There is actually no real review of any code at all. At least, not in terms of quality or security.
That's code bazaar!
It's a bizarre bazaar for sure.
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.
Not every codebase project etc use such workflow
Also such approach doesnt work with bug fixes / regressions
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.
a PR review is the final defence line before a QA
> If I work until I'm exhausted and find three bugs, someone might still complain later that I missed a fourth and I should have tried harder.
This is a weird take. Less bugs is less bugs, just because you maybe didn't find them all doesn't undermine the value of finding some.
Why are we still so obsessed about maintaining code when rewriting it is very low cost now?
It's kinda funny that we all agree that code review is important but we cannot agree why exactly it is important. Imagine a bunch of ancient shamans arguing why exactly local volcano needs to be fed one virgin a year but all being in agreement that it does.
> As everyone should know by now, it is not in general possible to find bugs by examining the code.
Lost me here. I would agree that it’s not possible to find every bug by examining code, but in real code reviews bugs and errors are identified by reviewers all the time. Reviewers lend their past experience to the situation, identify some unnoticed interaction, think of an edge case that the author hadn’t considered, or some times just notice simple logic errors.
Code review is a fresh set of eyes. When we write code eventually parts of it get accepted in our minds as done or correct and we can start missing things that are obvious to a reviewer.
I’m not a fan of these blanket declarations that code review isn’t about reviewing code. I’ve read countless hot takes like this that code review is about some other thing (finding unmaintainable code, knowledge transfer, etc) that all miss the point that code review isn’t about one thing. These reductions and exclusions can be really misleading.
I would add to this other purposes of code review:
* to share knowledge among the team — if code has been reviewed then at least two people know about it
* to ensure the changes won’t cause problems with other systems or future plans — maybe we will be rolling out a new logging system soon and we don’t want to solve the problem in this specific way
* a chance for the reviewer and reviewee to learn something about the system or the code via the review discussion
* team coherence — code that’s well reviewed is a team effort. Working together on small things like code reviews helps teammates work better together on other tasks in the future
I don't know about many people, but the author for sure doesn't understand the primary purpose of code review.
If the primary purpose of code review is to assess maintainability, there is no need for review, that can be done by automated tooling (formatting, bad naming, cyclomatic complexity etc.)
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.
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.
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.
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.
> many people misunderstand the purpose of code review
Oooh, I bet including the author? Yeah, right there, he fails to make any qualifications for his statement, making it factually incorrect.
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.
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!
> As everyone should know by now, it is not in general possible to find bugs by examining the code.
... I'd love to know what the author really meant to write here, because it certainly wasn't this.
> 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.
this. was a lead in small businesses working with very inexperienced people in one team -- literally having to teach them git -- and another with outsourced devs who had one day a week for us and could barely remember cos they context switched onto other projects for the other 4 days a week etc.
my job was to always know the most about the systems, what we're trying to achieve with the systems and what is in the codebases. answer questions. send links to people. know which devs to ask for more detail etc. i never necessarily knew everything about frameworks, libraries, package updates, new tools etc., but i knew our system/devs/goals/products inside and out.
as i said to several people, your first PR submission is just your first public draft. i will find bugs. i will find things that need changing. it's not personal. it's just about making your PR better than it was. this is an opportunity to learn from me.
the author of the article seems to be considering code review from an idealised situation -- this never occurs in reality, most of us have to make do.
and that means changing focus of code review for what is needed, rather than what is ideal.
He's a mathematician, so what he means by "in general", is "in every possible case", or "without exception", so what I think he means is, "not all bugs will be found by code review." I agree it probably could have been made more clear.
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.
Good programmers write code that’s so simple and obvious it looks like anyone could have written it.
Bad programmers make the simplest things really complicated.
Great programmers write code that makes you wonder why everyone doesn't do it like that.
Write me simple AI inference code that has high performance in big batches.
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 entire team, not only by the junior with something to prove.
Who is getting called at 2 AM when something breaks? Not the junior.
Hehe you think L9s do oncall? Senior not understanding is a problem all right. It's not necessarily junior's problem.
I feel like you keep missing the point and are more interested in gotcha's.
Does tinygrad not count for some reason?
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.
If that's how it functions where you work, I'd be looking for a new job.
There is a difference between how things are intended to be used and how they are used. Code review has been weaponized this way even at Google.