▲ 379 points
back
177 comments
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.
OP is Short-form false-dichotomy. Thank you for this perfect response.
Exactly - Readability Review [1] at Google was tedious but I generally liked the idea of having 2 separate discussions with different goals
1) https://abseil.io/resources/swe-book/html/ch03.html#readabil...
Similar to #3:
- having someone more experienced in that subdomain catch problematic parts and inconsistencies with existing code.
100% agree. It's as if you read the first sentence out of my mind. Thanks.
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.
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.
> 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
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.
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.
> 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.
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.
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.
Sure, ensuring maintainability is one of the benefits of code reviews, but I think it is a bold claim to say that's the solo purpose. For example, code reviews is also a tool that allows teams to get inform of the changes in the code and share responsibility of the whole code base.
The author is a mathematician, so when he says “it is not in general possible to find bugs by examining the code” he does not mean it is completely impossible to find bugs. He means only that it is not possible to find all bugs or even any particular bug.
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.
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.
> 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 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.
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 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.
This post is inverted for high assurance domains. For example, DO-178C requires checks for compliance to requirements, coding standards, traceability, accuracy and verifiability.
> 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.
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.
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.
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
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.
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 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.
> 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.
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 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.)
What if I told you that understanding what it is doing and finding bugs is actually the same problem?
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.
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.
I formed (and am advocating) a theory that code reviews are three dimensional space exercising trust, need for control and awareness propagation.
E.g. LGTM could be categorized as high trust, low control, high awareness.
I'm still in process of forming it on writing mostly because I get hooked up with pathological scenarios and these are infinite source of jokes to me. Some I written down already:
> 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.
The purpose of code review is to validate and socialize a change. Machines can get better and better at finding bugs, but until they are hooked up directly to our brains they cannot socialize changes because that involves human understanding.
> As everyone should know by now, it is not in general possible to find bugs by examining the code.
A code review doesn’t need to find every possible bug to be useful. Unit tests also don’t catch all bugs. They’re still a good idea.
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?
Found plenty of bugs by reading/doing code review.
"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.
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.
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.
100% this. This is why it's so hard to trust AI on code reviews, at least for now.