Everything has to be perfect. Everything has to be “clean”. It can really get quite stifling, especially when you remember a time before code review when working, high quality software was somehow still delivered.
IMO this system works pretty well as a balance between "we want to make sure nothing catastrophically bad hits production" and "we don't need perfect code, just good-enough code". It does require that you have a general understanding on the team that the purpose of the code is to deliver value to your users, and does not have to be perfect to be valuable.
We did also at one point try a system with more granular classification where every comment had to be prefixed with one of a predefined list of things like "nit", "security", "performance", "comment", or "kudos", which was fine but I don't think really adds all that much relative to just "blocking / non-blocking".
I had a friend describe to me how hurt he was because the code reviewer didn't like how he named things. Apparently he built a giant metaphor for his system. And he then wrote code in that metaphor.
I don't recall the details. It involved fruit, trains, clippers, etc. He was writing a system to report on how disk space was used across their systems.
That era also required more documentation effort since knowledge wasn't shared at the time the code was written and best practices weren't as well enforced around making sure code is readable since there was never a "test" of someone else looking at it.
Kind of feels like these folks are trying to distribute that QA role across the company (everyone dogfoods the latest versions). I imagine this would work well so long as the team is reasonably small and the product is simple enough that everyone uses every feature of the app often enough to find small bugs and corner cases, but I still wonder if at some point enough issues will arise in prod that could have easily been caught earlier such that someone will eventually say "maybe we should just look at each other's code and catch this stuff before it's committed?"
Would love to see a followup from these folks in another year or two.
The first kind should really just be linter rules. If it's not important enough to enforce across the project, it isn't worth mentioning now.
The second are strongly held opinions the other person doesn't want to make a big deal about. Maybe they are expecting pushback, or not feeling confident in their explanation of the issue. So they couch it in a nit.
You should be able to push back on comments you disagree with. Often I'll bring up suggestions that I think would improve the code but that I don't think are must haves. I think we can share a tremendous amount of knowledge in code review.
And of course, the easiest way to get fewer code review comments is to thoroughly review your own code with a critical eye. Review it like its someone else's. Find the typos and unclear variables. Anticipate the "nits" that you know are coming.
There was a time were a software developer was trusted as a responsible adult company employee.
I think that what changed is that, before, devs were mostly experienced trained professionals, and young ones were expected to be seriously trained before being "trusted".
Now, dev work has been commoditized with the influx of "web dev shops" and easy things like js and nodejs allowed a lot of mediocre script kiddies to be considered the same as professional developers.
After 5 weeks of debates and discussions, the original PR is merged with little to no modifications. But everyone lost time.
Sadly a lot of teams and companies see that as a performance metric (how measuring patches/commit measures how good somebody is programming?) or as a process that must be passed asap.
It's sort of a cross between a lightning talk, a "greatest hits of last week" session, or a demo.
"Last month I worked on X, and I had to do this hacky workaround. Here's why, here's how, here's the result, here's what you need to know if you need to work in this code soon.
Questions? No? Bob, you're up next with your work on Z..."
> Are extremely useful for onboarding new members on a project. You can learn a lot seeing others code, asking questions in place or receiving suggestions.
I expect utility of code review by a newbie/newcomer to the project to hover slightly over zero. Perhaps it's best to dedicate some time to proper onboarding instead.
A way to learn how to code well is to read (good) code.
You can dedicate time to onboarding, doesn't mean it's realistic that they're going to ramp up on all code areas during that time.
> I expect utility of code review by a newbie/newcomer to the project to hover slightly over zero. Perhaps it's best to dedicate some time to proper onboarding instead.
I disagree. If the commit isn't understandable to people, including junior engineers, I think it's problematic. I want to write code that is maintainable and not have to worry about deciphering my reasoning was for certain decisions down the line. If it's not clear to junior engineers, chances are it might not be clear to me, or someone else 3 years later.
Something that makes sense to me at the moment because I have context, might not be clear to others. Reviews are a great way to call that out.
Experience and competent does not mean perfect.
It's a very direct form of communication and also stops a lot of chaos especially when you're moving fast early on.
I can get behind a "no nitpicking" policy in some circumstances and always a "be kind" policy, but really someone should be looking over the push if only for their own edification.
I'm talking about less than a minute in many cases, a sanity check, because eventually someone will slip and commit debugging code that reveals sensitive information, outputs ":wq" in the header of every page, or something else that will be obvious to anyone who isn't the original author. Catching one of these before release is going to be worth doing hundreds of little reviews and a lot of pretty good automated test suites won't catch these kinds of bugs.
I'm in a large organisation that requires review for every change and I've certainly seen review cycles that took effort disproportionate to the level of importance and risk of the change. It doesn't happen every time, but it does happen.
Sometimes you can solve it by separately talking to the reviewer and pointing out the low risk or temporary nature of what you're doing, but I've also seen people occasionally getting focused on Enforcing Quality Standards and they can get annoyed if they feel that you're trying to bypass the process.
Huh.
Thats not for all code. Only the parts of code that need sox compliance. Plus its usually implemented as person doing the deploy is different from person writing the code. So yea code review isn't a requirement, requirement is that a single person cannot make change in the production.
I think as long as you have a good enough narrative about production change control (IE, review all code that is added to a release candidate, when the RC is about to be promoted) then you don't need code review of all code required.
That said, I find that in many cases, people make good code review comments and I've learned how to push back against unnecessary nits (at google, it's called "Ack.")
Tests should be assumed as a given with CD, so it is NOT just coding straight on live on production servers.
It's... not pretty.
Even with a team of experienced engineers we find bugs and mistakes in PRs daily.
Having reviews encourages knowledge of the entire system and why things work the way they do, or, why something was designed a certain way.
For a two person write and sell beta level code as fast as possible startup maybe this approach is fine, but mostly it is not.
Go slow and make things work/last.
I find this approach leads to resistance to fix all the mistakes the will undoubtedly get added over time.
Maybe because to fix those mistakes is an acknowledgement going slowly didn't pan out.
I love experimenting with new approaches and challenging old processes that everyone seems to do "because that's the way it's always been", but I feel too often we don't understand why a process like reviews are as popular as they are before we remove them. These processes grew organically, it wasn't some manager somewhere who came up with the idea. It was a solution to a human problem, and the hardware hasn't changed.
I find this is a great read if I'm not explaining myself clearly on my phone.
...that's why I like the Frost poem ("The Mending Wall") better the Chesterton quote. It typically gets read in a way that makes little sense, but the speaker took down a portion of the wall because the wall wasn't walling anything in or out. But he realizes while taking it down alone the same thing his neighbor knows and refuses to question..."Walls make good neighbors." (they hang out once a year, spending a day wearing themselves out to repair it, but the speaker can only recognize how hard it is to mend the wall, not how much he enjoys that day. Until he works taking the fence down by himself)
No one can truly do what the blog suggests. Humans are far less rational than we like to admit about ourselves. If they realize why the code reviews were originally implemented and change back the usual way of doing things, then the whole process works. Most of the time you gotta take down the wall to realize its real value (or confirm its lack of one)
> It's up to you and your colleagues to set the rules for your team. Don't adopt best practices in a dogmatic fashion. Rather, ask yourself if the circumstances of others apply to you. There is a high chance that they won't, and you find a more suited way to do things.
As OP scales his org he may find that the trust based approach no longer works and mandatory code reviews become the better approach. At that point it would make sense to introduce them.
I make mistakes. Sometimes there’s a typo. I sometimes miss that there was already a utility function that basically does that. Sometimes another engineer knows a better way to achieve the same thing. Sometimes I left in something unnecessary and missed it when I reviewed the changes.
I like having eyes on my code because it gives me reassurance and occasionally I learn things. It’s not an issue of trust.
* April 13, 2021 New command: Empty Trash
* April 28, 2021 Fixed an issue that would cause Empty Trash command to not work in some cases
Maybe you should have reviewed that code?
When I onboard junior engineers, this is one of the things I tell them to do.
It might help to see some patterns.
Also, I wouldn't like to be working for that company and then get something like a PagerDuty alert when you've no idea what has been released.
This just isn't correct. People make mistakes all the time. It's human. Code reviews can help catch some of them before they become bugs that need to be fixed. This has absolutely nothing to do with trust. If you have people who think that requiring code reviews mean that you don't trust them, you've hired some overly-sensitive people who have inflated opinions of their abilities and are ignorant of basic realities about software development.
I agree that some reviews end up being perfunctory (either due to a busy reviewer, or the trivial nature of a change), but as much as I am generally a process hater, I think code reviews should be generally mandatory, but with a culture of merging small, seemingly-safe changes without review, when soliciting one would be an unnecessary burden. Even then, everyone working on a shared code base should be opening PRs for those sorts of things, even if they just merge them immediately without comment. That way you can use PRs as a communication tool (to let others know what's going on; even more useful in a heavily-developed repository), and if someone really does want to look over something after the fact, they can do so.
Doesn't match my experience. There's two levels to this:
A good review can catch bugs directly. But even more important, it can dispute an obscure design, ask clarification for intent of ambiguous pieces of code, ask for added tests.
A hassle short term, but saves accumulated debt long term.
This is my pet peeve with code reviews. They revolve around (1) only reading the code, not pulling the branch and testing it, and (2) reading only the changes.
One time I was about to give an approval on a PR because the Python code looked fine. Then I thought "eh, maybe I should pull and test this branch, just in case."
With the new code in place, there was a marshmallow dataclass ValidationError in another part of the code that was not part of this change. This dataclass was used to ingest a JSON response from an API built by another team, and by default it had strict checking, including rejecting unknown JSON properties.
The developer had been testing against an older version of this API that didn't have the new properties, and I was testing with a more recent version. The other team seemed to follow what I think of as the "default JSON API contract" which is that any changes or removal of existing properties need to be coordinated with the consumers of the API, but they felt free to add new properties to the JSON object. And this broke the strict marshmallow validation.
The dataclass had been added some time ago, but this problem never arose because we weren't actually using this API until the new code in the PR started calling it.
The fix was simple enough; I just added this inside the dataclass:
class Meta:
"""Ignore unknown fields that we don't use."""
unknown = marshmallow.EXCLUDE
In the case of a JSON API response created by some loosely-coupled team, you're better off not wasting the time on that. Instead, keep the strict checks and unit tests on properties you rely on, and use the marshmallow.EXCLUDE to ignore new ones that you don't care about.
Whether you agree with that or not, this is the kind of bug that no one would ever catch in a "read the code changes" review.
It reminds me of the quote attributed to Donald Knuth: "Beware of bugs in the above code; I have only proved it correct, not tried it."
Code reviews become a necessity when you have really awful developers on your team. Fortunately I have not encountered many of them, but there was one developer that managed to make errors in even the simplest modifications.
In most environments, however, code reviews are worth it for the mentorship alone.
(I don’t argue against no code reviews (though I lean in favour).)
It seems the proposers of this might have had a dysfunctional or even toxic culture. Code reviews should be educational, safe, and fairly blameless. Also breaks knowledge silos.
Engineers that don’t like their code to be reviewed are a people smell.
I can't get behind this one. Sure, you can spin it this way, and maybe if you were in a traumatic environment with jerks as reviewers you might think this is a universalizable statement. But in a good team with good communications, pull requests are a good way of getting folks on the same page
> Pull requests don't prevent bugs.
Objectively false. (And trivially falsifiable; I'd bet every reader here has either found a bug in someone else's PR or had a reviewer find a bug in their PR in the last month. I know I have, on both counts.) They don't prevent _all_ bugs, but they do prevent some bugs. Lots of research on this, the general push for "shift left" is based on empirical findings that it costs less to fix a bug the earlier in your process you catch it.
If the author meant "don't prevent all complex bugs", then sure, but I feel that's an unreasonable demand for any process; you don't have to catch all bugs in order to provide a positive ROI.
> We rely on fast iterations with user feedback.
If your business model is not hurt by shipping bugs to users, then the value of fixing bugs before you ship is low. More mature products/companies don't really have this option.
> Pull requests slow down. Code reviews aren't high priority for engineers. They have to make time to clean up their review queue. In reality, PRs are an afterthought and not what you want to spend your time on as an engineer.
I think this is the actual problem. Many orgs have a code review process that makes it painful for developers to iterate quickly on their ideas. You can avoid this problem by not doing code reviews, or you can fix this problem by doing code reviews better. The latter is hard though! If you make "PR response time" a metric for your team, create a slackbot to nag on outstanding PRs, and generally as an engineering leader keep the focus on fast reviews as a priority, then you can make this a cultural strong-point instead of being a source of drag.
"Accelerate" by Forsgren et. al. talks a lot about 1) the value prop of, and 2) some techniques for, speeding up the cycle time from commit-to-production and idea-to-production. This is not easy, but I think in most cases (i.e. for most company/team sizes), dropping code reviews is throwing out the baby with the bathwater.
More generally, I think this post is falling into the (very common) trap of giving uni-directional advice; "reviews slow us down, therefore they are bad", rather than more nuanced analysis like "for our stage where quality is far less valuable than iteration speed, we think it's not a good investment of time and effort to build a strong mandatory reviews process". This is a fine position to take, though it's less snappy and I suspect won't get you onto the front page of HN. But you don't need to make claims about how code reviews damage trust or don't actually provide value to justify this tradeoff.
And if you're honest about why you're making the tradeoff, you'll be better placed to change the decision when the tradeoff falls in the other direction; a company that builds into its DNA the idea that "Code reviews harm trust" is going to avoid mandatory code reviews well past the point that they would provide positive ROI.
This is incredibly irresponsible.
Never skip code review if for no other reason than security.