Posted by ColinWright 6 hours ago
- 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 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
- having someone more experienced in that subdomain catch problematic parts and inconsistencies with existing code.
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.
This has been tried by a couple of my past managers.
This feels great with a small team on a slow moving codebase. If you try to force it on a larger team or expect the codebase to move quickly then it turns into a performative game of skimming the code (if that) to click the thumbs up button so you can get back to your work.
The end game was a situation where nobody was really reviewing code because everyone had their own work to do and they didn’t want to be the one person blocking important PRs, so everyone was clicking thumbs up.
Then again, if you're dealing with an un-maintainable minefield of a code base then "fire the whole team, I dare you" might be what you're after.
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
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?
or a prod outage causes the knowledge to be experienced.
At least, that's what people do by default.
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.
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.
Looks like you had both.
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.
Maintainability is a major factor in that, of course.
Our team started using AI, so I switched to a simple method: no comments, and a binary "is this batshit crazy or passable" approval decision rule.
Saving myself time and sanity.
I'll be really interested to follow what comes out of the Bun team.
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.
The purpose of code review is multi-faceted. Hard to maintain? Yes. Might have bugs? Yes. Can be done simpler/cleaner? Yes. Is in line with project code style? Yes. Get someone else to also understand the code? Yes. Onboard junior team member? Yes. Sanity check design decisions? Yes.
This flippant note is mostly more self-justification for being a lazy code reviewer.
- Does it functionally achieve what it sets out to (as per tacker issue or PR description)?
- Does it have extraneous code? Leftover debug prints, private API keys etc...
- Does it have any obvious defects? Memory leaks, un-handled edge cases, security flaws, obsolete API calls, etc...
- Could it be more understandable? Add/remove abstractions, better variable/method names, more/less functional etc...
- Is the style consistent with the codebase and/or style guidelines?
- Are there obvious performance improvements? Hashset instead of list, lazy evaluations, etc...
- Is it sufficiently well tested?
I'm not even sure I agree that if I can't understand the code then it shouldn't go through. Some code is just really hard to understand. The aim is to make it as easy as possible to understand, while being functionally correct.
Unfortunately this article is just bait, may as well say "People seem to think dinner is about eating food, but it's not about eating at all, actually it's about connecting with family and friends!". It's a specific type of poorly constructed reductionist argument that plays well on HN.
I've found the review and debugging process to be much more time consuming than writing/producing code, and just "praying it works" never ends well.
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.
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.
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.