10 ms·

No Code Reviews by Default

dekhn 5y ago
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? Huh.
- dominotw 5y ago
  > your SOX narrative doesn't cite code review 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.
  - dekhn 5y ago
    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.")
- koblas 5y ago
  SOC 2 compliance as well. If you cannot check the box that says "all changes are reviewed prior to release" you're SOL.
- scottshamus 5y ago
  Exactly what I was thinking… code reviews are a major component of many security requirements.
throwaway4good 5y ago
Looks good to me.
- avelis 5y ago
  I approve of this comment.
GhostVII 5y ago
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.
davedx 5y ago
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.
- zelos 5y ago
  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.
- nine_zeros 5y ago
  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.
- quartz 5y ago
  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.
avelis 5y ago
My biggest sticking point is that the team write tests where applicable for every PR submitted. Is it fullproof? No. Does it ask for trust? Yes. YMMV
palijer 5y ago
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. https://fs.blog/2020/03/chestertons-fence/ https://fs.blog/2020/03/chestertons-fence/
- CipherThrowaway 5y ago
  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.
- creaturelyme 5y ago
  > 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)
GGfpc 5y ago
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.
- thomaspaulmann 5y ago
  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. It might help to see some patterns.
estebarb 5y ago
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.
- varjag 5y ago
  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.
  - SpaghettiX 5y ago
    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.
  - NikolaeVarius 5y ago
    I'm always amazed by the arrogance of some coders. Experience and competent does not mean perfect.
    - varjag 5y ago
      Code review also does not mean perfect; in fact noone here has said anything about perfection.
  - makoz 5y ago
    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.
jrumbut 5y ago
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.
- chromanoid 5y ago
  I totally agree. It's like not copy editing a construction manual for ready-to-assemble furniture.
- SpaghettiX 5y ago
  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.
  - azornathogron 5y ago
    > 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.
  - jrumbut 5y ago
    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.
throwaaskjdfh 5y ago
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.
- valbaca 5y ago
  > would be similar to coding on the server Tests should be assumed as a given with CD, so it is NOT just coding straight on live on production servers.
  - morelisp 5y ago
    Seen at least two companies with > 15 programmers who did CD before comprehensive test suites and CI. It's... not pretty.
- chromanoid 5y ago
  It sounds like a good idea :)
- deleted 5y ago
  [deleted]
eCa 5y ago
It seems slightly off to argue no code review = trust, since that also argues code review = lack of trust. (I don’t argue against no code reviews (though I lean in favour).)
the_gipsy 5y ago
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.
lrvick 5y ago
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.
sitdown 5y ago
From the changelog: * 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?
- sinsterizme 5y ago
  Are you suggesting pushing code that was reviewed never has bugs either?
- pitnikola 5y ago
  I'm the author of that bug and I can assure you that no code review would catch it.
pcmoney 5y ago
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.
z77dj3kl 5y ago
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.
trhoad 5y ago
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!
- mxz3000 5y ago
  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.
netik 5y ago
Yeah, no. 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.
- postalrat 5y ago
  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.
- thomaspaulmann 5y ago
  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.
  - TheCoelacanth 5y ago
    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.
jefftk 5y ago
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.
Nursie 5y ago
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.
- postalrat 5y ago
  Everyone should be free to request reviews by default if they wish.
  - thomaspaulmann 5y ago
    That‘s the point. Everybody can, we just don‘t require it. Leaving it up to the individual developer to decide when it‘s right.
    - Nursie 5y ago
      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.
      - postalrat 5y ago
        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.
taylodl 5y ago
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.
- postalrat 5y ago
  Can you give an example of the test you would expect for a function that adds two numbers and returns the result? If you can't test simple thing properly how do you expect to be able to test more complex systems.
  - rand_r 5y ago
    assert add(2, 3) == 5 Seems simple enough :)
kelnos 5y ago
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.
secondcoming 5y ago
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.
- postalrat 5y ago
  I assume their job titles are "Co-founder".
Stratoscope 5y ago
> Pull requests don't prevent bugs. 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."
coderdd 5y ago
> Pull requests don't prevent bugs 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.
theptip 5y ago
> Pull requests discourage trust. Proposing every code change that somebody else has to approve doesn't feel encouraging... 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.
aliasEli 5y ago
I think that in general a good idea to have code reviews with new team members. It gives a good handle to talk about code organization, procedures and team conventions. You can also the new member to review modifications from other team members. 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.
sys_64738 5y ago
Code reviews keep developers honest. Left to their own devices, developers will employ their terrible habits which sneak into the code base. Code reviews isn't just about reviewing the code but it's raising the bar for what is actually presented as complete. It separates sloppy code from complete, fully tested code. Developers are lazy and will look for shortcuts if standards are not maintained.
pixel_tracing 5y ago
Future: * Breaking News: Raycast shuts down, a spokesperson from the Raycast team had this to say; “It was.... oh god.... oh no! If we only reviewed that code!”
ManlyBread 5y ago
I've worked in a place that didn't do code reviews. The argument here was that you're supposed to think about every case and ask questions even for what seems to be a trivial matter. That was the "official" explanation, the reality was that the boss was dumb enough to claim that "we don't have time and the client doesn't pay us for code reviews". This has continously lead to more bugs and comments like "what is this, why is this code here, who did it this way, it makes no sense". It also turned deployment into a nightmare since there always had to be at least two people on standby in case some piece of code starts wrecking havoc in production. I have managed to successfully convince the people working there to change the approach by forgetting a WHERE clausule in an SQL script which has caused havoc in the production system and forced several people to work at night and on the weekend. No one has caught it in testing because the change - despite the severity - was tested by a very inexperienced person and the testing environment has almost no data in the database so no one noticed the problem at this stage. I don't feel bad at all about this because the change was big and I actually asked for review (even though we had no such process) and I was ignored. It also wasn't the first time something like this was happening at that company, it was like that for years. The havoc could be even worse if I made a similar mistake somewhere else in the script, it would take weeks to clean up the mess and it would no doubt cause a huge financial loss for the client. There were some people who actually quit that company over the lack of such oversight and so did I. I think it's an important failsafe mechanism, no less valuable than unit tests for example.
- nsonha 5y ago
  I'll give no CR with other strong engineering practices the benefit of doubt, but from story it's likely no review no automated tests which sounds like hell.
  - ManlyBread 5y ago
    Yeah, any automatic tests were optional and took ages to execute due to the technologies and approaches used. Running around 1k tests on Ryzen 3700X was taking around 30 minutes.
eithed 5y ago
I'd want somebody else to look at my code - the more I spend on my code the more blinded I become to the issues I'm trying to fix while sacrificing others.
hulitu 5y ago
Looks like Microsoft or Google. We wrote the code, the user shall test it.

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? Huh.

dominotw 5y ago
> your SOX narrative doesn't cite code review 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.
- dekhn 5y ago
  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.")
koblas 5y ago
SOC 2 compliance as well. If you cannot check the box that says "all changes are reviewed prior to release" you're SOL.
scottshamus 5y ago
Exactly what I was thinking… code reviews are a major component of many security requirements.

throwaway4good 5y ago

Looks good to me.

avelis 5y ago
I approve of this comment.

GhostVII 5y ago

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.

davedx 5y ago

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.

zelos 5y ago
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.
nine_zeros 5y ago
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.
quartz 5y ago
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.

avelis 5y ago

My biggest sticking point is that the team write tests where applicable for every PR submitted. Is it fullproof? No. Does it ask for trust? Yes. YMMV

palijer 5y ago

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. https://fs.blog/2020/03/chestertons-fence/ https://fs.blog/2020/03/chestertons-fence/

CipherThrowaway 5y ago
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.
creaturelyme 5y ago
> 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)

GGfpc 5y ago

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.

thomaspaulmann 5y ago
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. It might help to see some patterns.

estebarb 5y ago

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.

varjag 5y ago
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.
- SpaghettiX 5y ago
  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.
- NikolaeVarius 5y ago
  I'm always amazed by the arrogance of some coders. Experience and competent does not mean perfect.
  - varjag 5y ago
    Code review also does not mean perfect; in fact noone here has said anything about perfection.
- makoz 5y ago
  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.

jrumbut 5y ago

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.

chromanoid 5y ago
I totally agree. It's like not copy editing a construction manual for ready-to-assemble furniture.
SpaghettiX 5y ago
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.
- azornathogron 5y ago
  > 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.
- jrumbut 5y ago
  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.

throwaaskjdfh 5y ago

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.

valbaca 5y ago
> would be similar to coding on the server Tests should be assumed as a given with CD, so it is NOT just coding straight on live on production servers.
- morelisp 5y ago
  Seen at least two companies with > 15 programmers who did CD before comprehensive test suites and CI. It's... not pretty.
chromanoid 5y ago
It sounds like a good idea :)
deleted 5y ago
[deleted]

eCa 5y ago

It seems slightly off to argue no code review = trust, since that also argues code review = lack of trust. (I don’t argue against no code reviews (though I lean in favour).)

the_gipsy 5y ago

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.

lrvick 5y ago

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.

sitdown 5y ago

From the changelog: * 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?

sinsterizme 5y ago
Are you suggesting pushing code that was reviewed never has bugs either?
pitnikola 5y ago
I'm the author of that bug and I can assure you that no code review would catch it.

pcmoney 5y ago

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.

z77dj3kl 5y ago

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.

trhoad 5y ago

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!

mxz3000 5y ago
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.

netik 5y ago

Yeah, no. 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.

postalrat 5y ago
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.
thomaspaulmann 5y ago
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.
- TheCoelacanth 5y ago
  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.

jefftk 5y ago

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.

Nursie 5y ago

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.

postalrat 5y ago
Everyone should be free to request reviews by default if they wish.
- thomaspaulmann 5y ago
  That‘s the point. Everybody can, we just don‘t require it. Leaving it up to the individual developer to decide when it‘s right.
  - Nursie 5y ago
    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.
    - postalrat 5y ago
      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.

taylodl 5y ago

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.

postalrat 5y ago
Can you give an example of the test you would expect for a function that adds two numbers and returns the result? If you can't test simple thing properly how do you expect to be able to test more complex systems.
- rand_r 5y ago
  assert add(2, 3) == 5 Seems simple enough :)

kelnos 5y ago

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.

secondcoming 5y ago

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.

postalrat 5y ago
I assume their job titles are "Co-founder".

Stratoscope 5y ago

> Pull requests don't prevent bugs. 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."

coderdd 5y ago

> Pull requests don't prevent bugs 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.

theptip 5y ago

> Pull requests discourage trust. Proposing every code change that somebody else has to approve doesn't feel encouraging... 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.

aliasEli 5y ago

I think that in general a good idea to have code reviews with new team members. It gives a good handle to talk about code organization, procedures and team conventions. You can also the new member to review modifications from other team members. 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.

sys_64738 5y ago

Code reviews keep developers honest. Left to their own devices, developers will employ their terrible habits which sneak into the code base. Code reviews isn't just about reviewing the code but it's raising the bar for what is actually presented as complete. It separates sloppy code from complete, fully tested code. Developers are lazy and will look for shortcuts if standards are not maintained.

pixel_tracing 5y ago

Future: * Breaking News: Raycast shuts down, a spokesperson from the Raycast team had this to say; “It was.... oh god.... oh no! If we only reviewed that code!”

ManlyBread 5y ago

I've worked in a place that didn't do code reviews. The argument here was that you're supposed to think about every case and ask questions even for what seems to be a trivial matter. That was the "official" explanation, the reality was that the boss was dumb enough to claim that "we don't have time and the client doesn't pay us for code reviews". This has continously lead to more bugs and comments like "what is this, why is this code here, who did it this way, it makes no sense". It also turned deployment into a nightmare since there always had to be at least two people on standby in case some piece of code starts wrecking havoc in production. I have managed to successfully convince the people working there to change the approach by forgetting a WHERE clausule in an SQL script which has caused havoc in the production system and forced several people to work at night and on the weekend. No one has caught it in testing because the change - despite the severity - was tested by a very inexperienced person and the testing environment has almost no data in the database so no one noticed the problem at this stage. I don't feel bad at all about this because the change was big and I actually asked for review (even though we had no such process) and I was ignored. It also wasn't the first time something like this was happening at that company, it was like that for years. The havoc could be even worse if I made a similar mistake somewhere else in the script, it would take weeks to clean up the mess and it would no doubt cause a huge financial loss for the client. There were some people who actually quit that company over the lack of such oversight and so did I. I think it's an important failsafe mechanism, no less valuable than unit tests for example.

nsonha 5y ago
I'll give no CR with other strong engineering practices the benefit of doubt, but from story it's likely no review no automated tests which sounds like hell.
- ManlyBread 5y ago
  Yeah, any automatic tests were optional and took ages to execute due to the technologies and approaches used. Running around 1k tests on Ryzen 3700X was taking around 30 minutes.

eithed 5y ago

I'd want somebody else to look at my code - the more I spend on my code the more blinded I become to the issues I'm trying to fix while sacrificing others.

hulitu 5y ago

Looks like Microsoft or Google. We wrote the code, the user shall test it.