7 ms·

If something hit production and caused a major fuck-up because there was no peer review process, then in all the places I've worked at the first action item in

• jolux 5y ago
  Causing a major fuck-up in production is probably also a sign that you need better release validation and deployment practices.
  • jaredsohn 5y ago
    For many developers, people merge their branches directly to production so the pull request review is where that checking happens.
    • watwut 5y ago
      Ok, but many companies don't use such ridiculous process.
      • dtech 5y ago
        iirc Google does it, it's not ridiculous at all. You need a lot of automated tests and canary deployment to pull it off though.
        • joshuamorton 5y ago
          Not really. Googler merges to HEAD, but systems aren't running directly from head.
      • kcb 5y ago
        It's not as ridiculous as it sounds as long as the testing and verification happens as part of the PR.
        • saurik 5y ago
          Sure, but then you are just begging the question: if you have a better process for release verification you might not be need any per-commit code review at all.
      • jaredsohn 5y ago
        If you want to learn more about this, read up on 'continuous deployment'. I remember when I was first exposed to it it seemed dangerous/scary but with the right culture changes it can work really well; lots of companies do it.
    • jolux 5y ago
      Sure, I mostly meant automated processes. They won't always save you from yourself but doing blue-green, having an extensive test suite, etc, are all things that will help reduce the risk of a bad deployment.
      • lewisl9029 5y ago
        Yeah exactly, code review isn't a good place to catch actual bugs. Humans are terrible at catching bugs consistently... Humans probably introduced those bugs in the first place. It's also an exceedingly poor use of human time, which costs a heck of a lot more than machine time. We really should be relying on automated systems to catch bugs for the most part, and leave code review for what it's good for: making sure a different human can understand and maintain the code. In fact, if we manage to catch a bug in a code review that didn't also trigger our automated systems to alert us, that should probably prompt us to look into how we can expand the coverage of that automated system, similar to what we'd do in a postmortem if the bug wasn't caught in code review and reached production, because we can't rely on that happening the next time.
        • munchbunny 5y ago
          How do you systematically (and proactively) ensure your testing is thorough? With testing you often run into unknown unknown cases. I’ve always code reviewed/gotten code reviewed on both the implementation and the tests, with completeness as the main quality you’re reviewing test code for. If that’s how we do it, we’re right back at code reviews.
          • lewisl9029 5y ago
            That's definitely an unsolved (unsolvable?) problem. The best we can really do is start with a common sense set of initial tests based on the user observable behaviors we want from the system and then add more tests organically as we learn more about the system's edge cases through incidents, bugs and regressions. I'm not saying code review can't find any bugs (in practice bugs are found in code review all the time), I'm just saying that if we do find a bug through code review, it means we got lucky, and we shouldn't count on getting lucky again, so better add a test/linter rule for the next time when we might not get so lucky. That's just one more way to grow your automated systems organically.
• alkonaut 5y ago
  Exactly. I want my code peer reviewed partly because it makes it clear and formal that while we succeed as a team we also fail as a team. It’s much easier to talk about failures when a failure doesn’t have a single person attached to it.
  • mpweiher 5y ago
    > I want my code peer reviewed TFA: > Engineers [..] request reviews when they think it's necessary. Problemo solved.
    • pbalau 5y ago
      Good engineers will ask for code review. Most engineers won't.
      • mpweiher 5y ago
        > Good engineers will ask for code review ...when they need it. Actually, really good engineers will ask to pair when they need it. Review is a distant second.
        • btschaegg 5y ago
          > Review is a distant second. I agree with you there, but I'm also sure that one reason for this is that many people still see reviews as the task that only happens after everything else. I'm convinced there's a lot of useful middle ground to be had if reviews aren't just "here's 500 LOC, please review", but maybe more… staged. Say: Rummage around in the code a bit for yourself (ask questions if you have to), come up with a strategy of solving your problem, and submit that for review. Not in code, but maybe using markdown, preudocode, small diagrams or the like. In person (i.e. via screenshare nowadays). And if the other party doesn't find obvious holes in your strategy, then go implementing it. I remember an interview with some Linux kernel maintainer that mentioned the same effect -- people submitting a gigantic patch for something that will simply never be merged. And all that would have been necessary to avoid this wasted work was an e-mail asking if the maintainers are at all interested in the change.
• rdw 5y ago
  This post sounds like wisdom, but after looking at it hard, it seems that the point is no more than "peer review processes are popular". Not to say that code review is bad in any way (I do it at my current role), but, this argument for them is not very good. One downside of code review is reduced velocity. There are teams out there who use code review so effectively that they never ship big fuck-ups to production. They never have to tell clients that they have bad practices. Instead, they struggle to get clients because they pay much more per line of code shipped and therefore don't deliver as good of a value as their competitors. This where the argument needs to be: whether and how to have code reviews be a net positive.
  • ljm 5y ago
    Ultimately I think it's down to where you want to pay for that decision (for want of a better term), or where to put the bottleneck. I think that putting the bottleneck in production (which would block all work unrelated to a fix if a catastrophic fuckup took place) might as well be a hedge against the possibility of things fucking up so badly in a given timeframe. You could also make code review a bottleneck, but I haven't seen that work well in practice compared to putting it at QA or as part of a release process (unless you combine them in a continuous delivery workflow). I can see how that equation balances in favour of no code review when it's in the context of a startup that is probably still figuring out how to make money, especially with a subscription based app launcher. As far as prototyping and early stage iterations go, you're probably just seeing what works and reviewing the code isn't so important at that point. There would still be one or two things you'd ask someone to check, e.g. security related stuff. I think you make a good point though: what makes an effective code review process? I don't think there's a single answer to that because it depends on the circumstances. I would at least say that it goes better if it's treated as a priority and issues are raised and dealt with promptly, rather than leaving it as a chore you eventually get around to. And you would have to see value in it beyond it being a sanity check, as others in this thread have described (knowledge transfer, for example).
  • baron816 5y ago
    I'd argue that social proof often makes for a good argument. Perhaps not the best argument, but still a good one. "Best practices" still carry weight. Of course you can make an argument that the thing everyone does is wrong, and win. That's happened millions of times in this industry. But if the argument is nearly strictly between "this is what everyone does" vs "this is what I do", the former is going to win.
• deleted 5y ago
  [deleted]
• quartesixte 5y ago
  > For me, it's been hard to go back to my old, cavalier attitude after that. I’ve heard from my workplace’s software team that this makes hiring from certain sectors particularly difficult because they don’t get this. Too used to move fast and break things and MVP.
• sharken 5y ago
  It's the same in the financial sector, you can't just push code without a review. However, the review process is far from perfect and can create a false sense of security. To review a piece of code, that code should ideally be small in scope. For larger pieces it's rather common that the reviewer don't have enough time to do a good job. If there were better incentives for doing a review, then it would greatly improve the situation. But i haven't seen that yet.
  • XorNot 5y ago
    I've had a lot of trouble getting people to do proper reviews. A minimum to me is that you actually compile and execute the code in some way to check its sane. Better would be the reviewer actually adds to the test suite for the code to prove their expectations of how it works. In almost all cases it's very hard to get people to look outside the web browser for the diffs. Diffs show you something, but never the whole context. The other problem I've found is stamping down on style arguments: they have no place in code reviews, and a lot of the time you get a round of "please change this variable to be named something else" - which might be valid, but raises the question of why the reviewer didn't do it, and more often leaves a weird authority gap - I have no power to just reject such a request because I think it's invalid, nor does the business have anyway to resolve the issue. In fact if there's a dispute at all, very rarely is there any process for bringing in a third party to mediate or break the stalemate (which is hilarious in some ways since everyone by now should realize 2-party systems can't achieve consensus). Code reviews are very, very cargo-culty and I've not seen them ever include a thorough design: people give up on the very near edge cases, and so the whole system falls apart because it's just a hurdle to get through and not a collaborative or constructive process.
    • kqr 5y ago
      Why should it be on the reviewer to compile and execute the code, rather than the author? Or are you saying that one can actually find more bugs when two people compile and execute the same code rather than just one? When I review code, I want the author to tell me how they have verified that the code has the desired effect -- but if they do that, I'm going to trust that they did the actual verification and I'm not going to simply retrace their steps. That seems like a waste of time to me.
• hsn915 5y ago
  This strikes me as weird excuse for code reviews. To spot quality problems before going to production .. what you need is testing. Not "unit testing" - which is just another pointless process that does not actually provide the purported benefits. You need a separate QA team that handles testing - manual and automated.
  • kevingadd 5y ago
    If you don't catch major errors until the QA team has time to do a full comprehensive test of the product then those errors are probably sitting in the codebase for days or weeks, and additional changes are being piled on top of them which will make it harder to identify which commit was the problem. Do a code review and you'll catch a bunch of stuff before it even hits QA.
    • pc86 5y ago
      Major errors are easy to spot and will likely take QA minutes to find. They're also probably not going to do a full test of the entire system with every change unless you've got a very small product. One thing I've seen that I like a lot is individual changes creating individual front ends. You test the one thing in that change, and it's done. This tends to cause a lot of problems with CORS, but then what doesn't? Agreed with you though that good code reviews (whatever that means) obviates most of this.
      • kqr 5y ago
        The actual job is always fast. What takes time is for the job to sit in the queue outside the QA department. The figurative papers moving between desks is always what kills your efficiency, not people doing work slowly.
    • kqr 5y ago
      Correct. Any time you need a completely separate team to handle any aspect of the lifecycle of the product you're introducing delays and miscommunication. Besides, a separate team to do a part of the job scales badly: it needs to grow at least linearly with the number of developers, sometimes superlinearly. You should absolutely have a small QA team, but their job shouldn't be doing the QA -- it should be helping others do their own QA.