So... code review isn't part of your annual security audit? Specifically, your SOX narrative doesn't cite code review as a method used to prevent single-party attacks?
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 most lawyers look at this and say "all code needs sox compliance" because you can't easily guarantee some bit of code won't end up being checked into a server binary that runs in prod at some point.
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.")
I think this pretty clearly wouldn't scale very well unless you have incredibly good hiring practices, and complete testing coverage. But it is interesting that it works for them at their scale.
As someone who worked on code bases with sometimes fairly big teams before the days of mandatory code reviews: I applaud this. I feel like the culture of code review has become way too serious and strict, to the point we need a “Nit:” syntax because yes, people take issue with the very biggest and very littlest parts of your contributions these days.
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.
Most of the Nit level issues should probably be caught by the linter/static checker, which reduces a lot of the minor comments.
Not that that seems to stop some people, of course.
Nitpicks by devs with incomplete information are the worst. They have little context but end up blocking a PR for weeks because they think it should be solved differently or are not convinced.
After 5 weeks of debates and discussions, the original PR is merged with little to no modifications. But everyone lost time.
Not sure of your personal experience with this but mine was that the era before code reviews was mostly enabled by having often large teams of QA / SDETs who were burdened with finding the output of bad code and throwing bugs back at the eng team (see: the famous account of winword engineers pushing known bugs into the code to increase perceived velocity with the expectation to get a QA report back to fix them).
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.
Yes and no. If you have mostly senior people who take responsibility for the code they write, then the amount of stuff getting picked up by QA will be more or less the same IME. Code reviews catch some stuff but they can also slow down your team’s velocity disproportionately to the bug/arch issues it catches. YMMV of course. Some orgs have much more sane and pragmatic code review practices than others.
At my workplace all code review comments must be classified as either "blocking", which is stuff like "this code will break the app for our users" or "this is insecure", or "non-blocking", which is stuff like "this method is hard to read and should be broken up" or "I think this variable could be named better". Non-blocking stuff is (really, truly) optional to address.
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.
Names are important for understanding code. They are most useful if everybody uses the same conventions. It sounds like your friend was trying to push personal conventions onto others.
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.
And the pendulum swings yet again. Eventually, with scale, I feel this approach will realize why the code reviews were implemented in the first place.
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.
I don't think the article says that requiring code reviews is never good process. Consider this last paragraph:
> 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 feel this approach will realize why the code reviews were implemented in the first place.
...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)
The fact that people open PRs because they are working on a new part of the code means that there's knowledge silos in the team that could be easily solved with code reviews.
Most codebases that I‘ve worked on are too big to know everything. Often it isn‘t even required to build a feature. I think it‘s unavoidable to touch new parts of a growing codebase and you are probably not a good reviewer in the first place if you don‘t know the part of the codebase.
I'm not really convinced. Code reviews are a great opportunity for learning, improve and share knowledge. 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.
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.
What if most of the team are already competent, experienced programmers? Sure there's always something to learn but perhaps not as much on a routine code review.
> 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.
I would think that GP meant review of the new joiners code, not the new joiner reviewing other peoples code. But even a new joiner reviewing existing code can be beneficial (to the new joiner definitely).
A way to learn how to code well is to read (good) code.
If the code review is routine, the approval will be quick and straight forward. There's barely any friction in my experience. I'd like to believe I'm competent, doesn't mean I'm infallible.
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.
My current team has very few code reviews, and recently we discovered we can recover a fair amount of missing learning opportunities with weekly or every-two-weeks having an (optional) 1 hour knowledge sharing session.
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..."
When OP's approach is implemented effectively code review does not disappear but becomes more of a continuous process. Developers review each others changes and become proactive about seeking review of their own code. Teams that take this approach will typically review all code changes between releases before code makes it into production.
I'm happy this works for them but I find so much value, even in a two person early stage team, of having someone take even the quickest of glances at your code.
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.
The post did not say "don't do code review", it said "do not require code reviews". This is different. However, I do think its nitpicking, because if a code review really doesn't matter, then the approval will come quickly and easily because of how easy it was to review and how little it matters: developers would not spend effort reviewing something that doesn't matter. Or is that a bad assumption I'm making which the post does not.
> developers would not spend effort reviewing something that doesn't matter. Or is that a bad assumption I'm making which the post does not.
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.
I'm making the case against making code reviews optional by relating my experience that there's a significant benefit to someone taking a quick glance when they don't have the time/inclination to do a thorough review.
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.
An approach that combined "no code reviews" with "continuous deployment" would be similar to coding on the production server, but with a better audit trail and consistent deployment across machines.
Seems out of question because companies are obsessed with minimizing risk. Instead, development crawls at snail pace because it takes way too long to integrate patches and too many developers get distracted with nitpicks. Or they just blindly approve, and PRs are just an extra timesink without any benefits. The developers hand off ownership at git push, everything after is out of control, bureaucracy , or entirely someone else’s job.
Without code review a compromised workstation can push malicious code that will make it to production and compromise all user data with little chance of being noticed.
This is incredibly irresponsible.
Never skip code review if for no other reason than security.
From my understanding code reviews are one of the few rituals in modern software development that has been empirically shown to provide benefits.
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.
Pitting code review against trust and responsibility is just disingenuous. The point of code review is not that you distrust others. It's that everyone makes mistakes and often someone else having a quick look over your changes can come up with some really cheap wins that are safer, more maintainable, faster, etc.
Hell, I code review my _own_ code before anyone else does. The amount of issues I've spotted simply by viewing them as a PR in Github is insane. Code-blindness is a real issue!
100% this. You can fix lots of small issues that would otherwise just be noise on the PR just by reviewing it yourself beforehand, as if it was somebody else's code!
When I onboard junior engineers, this is one of the things I tell them to do.
We aren‘t two engineers anymore, we are seven. We still apply the same processes. I‘ve worked with huge codebases where you often you can’t know the entire system. Focus on what to build and do it well. If you touch something new, collaborate with others who know the part of the codebase. Either through PRs, pair programming or anything else that gives you confidence.
Seven is still a pretty small team. One of the apps I'm working on has commits from 400 different developers over the past few years. It would be an absolute disaster without code reviews.
You want process that is appropriate for your situation. I've worked in environments where code reviews and even tests did not make sense, primarily ones where development speed matters above everything else and the risk of something broken is low.
In most environments, however, code reviews are worth it for the mentorship alone.
I’m an experienced programmer and I hope a pretty good one. At least I’m paid that way so maybe it’s true.
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.
Then IMHO you're likely to get an odd culture of people requesting reviews or not based on perceived urgency, and also you're likely catering to those who consider it a point of pride not to need them, which is not a good attitude.
We don't insist on review because we don't trust people, we insist on it because it catches problems, spreads knowledge around and helps developers.
IMO the biggest advantage for not requiring code reviews for every change is to concentrate code review efforts to where it's needed most: changes with higher risk.
Sure, every change has some degree of risk. But obviously some changes have more and if you are treating all changes equally then you may overlook the important changes.
I'm not sure that requiring a review implies treating all changes equally, but it does at least get some eyes on everything, and IMHO that's important.
Haven't done required code reviews in over 15 years. No need to. I care more about your tests and how your tests impacted how you wrote your code. That tells me everything I need to know.
The article makes a big deal about the idea that code reviews show a lack of trust, and that not having mandatory code reviews shows that people trust each other.
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.
I'd like to see their job titles, because if you don't do reviews you don't need to pay for senior engineers. Part of being a senior engineer is taking responsibility when broken code is released.
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 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
You might ask, "Shouldn't this problem have been caught by a unit test?" But realistically, who is ever going to write a unit test that says "let's add some random new property to the mock JSON response and make sure it doesn't raise an exception"? Especially when there are some 50 API calls like this.
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."
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.
88 comments
[ 4.7 ms ] story [ 155 ms ] threadHuh.
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.")
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.
After 5 weeks of debates and discussions, the original PR is merged with little to no modifications. But everyone lost time.
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.
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.
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.
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.
https://fs.blog/2020/03/chestertons-fence/
> 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.
...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 might help to see some patterns.
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.
> 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.
Experience and competent does not mean perfect.
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.
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..."
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 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.
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.
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.
(I don’t argue against no code reviews (though I lean in favour).)
This is incredibly irresponsible.
Never skip code review if for no other reason than security.
* 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?
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.
When I onboard junior engineers, this is one of the things I tell them to do.
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.
In most environments, however, code reviews are worth it for the mentorship alone.
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.
We don't insist on review because we don't trust people, we insist on it because it catches problems, spreads knowledge around and helps developers.
Sure, every change has some degree of risk. But obviously some changes have more and if you are treating all changes equally then you may overlook the important changes.
Things will get pushed without review that are considered urgent, bugs will slip through to live systems, bad time all round.
If you can't test simple thing properly how do you expect to be able to test more complex systems.
Seems simple enough :)
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.
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 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:
You might ask, "Shouldn't this problem have been caught by a unit test?" But realistically, who is ever going to write a unit test that says "let's add some random new property to the mock JSON response and make sure it doesn't raise an exception"? Especially when there are some 50 API calls like this.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."
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.