14 ms·

He mentions "trust" a lot. Is that why we do code reviews, because we don't trust each other? I find this attitude problematic. We do code reviews because huma

• islon 5y ago
  I have an open source project in which I work alone. I never work directly on master, always on a branch, and when I'm done I create a PR for myself. Can't remember a single PR that I reviewed where I didn't find some issue or bug. I guess you can say I don't trust myself...
• ljm 5y ago
  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 the post-mortem would be "we should introduce peer review." Otherwise someone would ask how we could ensure it wouldn't happen again, and no one would be able to say "because we trust them," and leave it at that. It would sound more like "I trust you will never make that mistake again," which sounds more like a threat. You also can't go to your clients and say "sorry, we let our team deploy stuff without reviewing because we trust them... so please continue to deal with our avoidable problems." Thing is, I've lead my fair share of teams and I trust my team-mates and colleagues implicitly. It makes for a strong team. But we still do code review (peer review) because we want to build software that works well and support each other. We're not in the job to simply deploy code to prod as quickly as possible. I'd also add that I think any software engineer should try and have the experience of working in a highly regulated field, like healthcare. It's hard to get an appreciation for why these things exist until you realise you're held to a much higher level of accountability because your oopsie moments can have much larger consequences. For me, it's been hard to go back to my old, cavalier attitude after that.
  • 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.
        • 5y ago
• mumblemumble 5y ago
  We (at least should) also do code reviews in order to make sure everyone understands the code. On my team, the majority of code review comments are not discussing potential bugs, they're making sure everybody knows how the new thing works, why it was designed that way, implementation tradeoffs, etc. All that discussion is extremely valuable over the long run. For example, it means that nobody has to pay attention to email while they're on vacation in case something goes pear-shaped with a module they implemented. That said, I 100% agree with the article's concluding paragraph about YMMV. Raycast is operating in an entirely different business domain from the one I'm in. It sounds like the reasons why Raycast likes their way of doing things don't really apply to us, and I don't think the reasons why we like our way of doing things don't really apply to Raycast.
  • rebeccaskinner 5y ago
    > We (at least should) also do code reviews in order to make sure everyone understands the code. This is the most common reason I see cited for code reviews, but I'm not entirely sure if I buy it, at least not for a lot of teams. In most cases you are going to have one, maybe two other people looking at a given PR. The positive way of looking at this is that you've just doubled (in some cases tripled) the number of people who are familiar with the code. In reality though, this is still going to be a very small fraction of most engineering organizations. If you have a small team who wholly owns a small codebase then it could be meaningful, but a lot of the time you're still talking about a rounding error from zero people knowing the code- most people in the organization will still be touching the code in the future without any prior knowledge of it, and will have to start understanding it from zero. Even if you are dealing with a small enough code base, in a small enough team, that the reviewer is adding a substantial margin of familiarity with the codebase to the team, reviewing the code is only going to give an extremely shallow level of understanding. The reviewer isn't going to have all of the context that the original developer had. They won't know all of the things that weren't done or why, the tradeoffs that were made, requirements, business context, etc. Best case scenario, someone wrote that down in a well-known place (comments in the code near the feature, something relevant in a docs directory, hopefully not as an archived jira issue that will never be looked at again). The reviewer probably has their own work to do anyway, and once they approve the PR they'll flush most of what they had out of their short-term memory anyway. In a lot of ways, this is the same set of reasons that people aren't very likely to find bugs in a code review either. Someone without a lot of context on the problem isn't likely to find many business logic errors, or missed requirements. Since you were busy working on your feature, you probably don't know who else on what other teams might have been doing something relevant to your work, so you wouldn't know to get a review from people who might have something useful to contribute (and they wouldn't know to proactively review your code). One of the reason senior engineering roles often devolve into telling people to talk to one another is to try to address this kind of situation, but that only scales up to a small fraction of the overall code churn at organizations of any size. For what it's worth, I ask for PRs, and I review my co-workers code. I make comment when I see something that might be useful, and I graciously accept feedback and try to make sensible decisions about when to make changes and when to let the feedback go. I actively search for PRs that might touch sections of the code I've been working in or am very familiar with to try to optimize the quality of the feedback I can give. I'm not really convinced any of this actually helps deliver better code in the moment, but maybe it does some good as a way to help teach my co-workers about things (or to learn about things they can teach me), and anyway it's such an ingrained part of engineering culture that it doesn't seem worth the political capital you'd have to spend to try to change it, and even if you succeeded it would be a prime target for scapegoating if anything went wrong later. It does slow down the development process I think, but people need down time anyway. If waiting on a code review gets people an hour or two of downtime to check their email or read HN between tasks then maybe it's actually worth it for that. The burnout caused by superfluous process and the occasionally large pile-ons and people demanding poorly thought out changes, gatekeeping your ability to deliver business critical work is essentially a cost of doing business that we seem to have collectively accepted in software. I should also note that I also work on some open source projects, and in that setting PRs work much better. Not only does such code often need more review, but you also typically have a smaller group of people who know the whole codebase, and are much better central repositories of knowledge. The review process is also a much more effective way of doing collaborative decision making in an open source project IMHO.
• alecbz 5y ago
  Yeah, IMO this is like a publisher saying "we don't have editors: we trust our authors". The intended ethos of editors and code review is that mistakes and imperfections are the norm, and you need a second pair of eyes to iron them out. That said, I do sometimes encounter individuals or cultures that seem to view code review more as a mechanism for catching abnormal/unexpected mistakes than as a normal part of the process of writing code.
  • jiggawatts 5y ago
    Something that I've discovered is that as the volume of text that I write goes up, the errors increase. More experience hasn't resulted in fewer typos. On the contrary, I make far more small mistakes now than I used to. It's precisely because I've learned to read and write so fast that I "skip over" the simple parts of sentences, making me oddly blind to certain categories of spelling errors! Especially when I've gone over the same text over and over, I'll have memorised it to the point that I can skip entire sentences. This is why you need someone else to step in and read the text for the "first time" and not skip anything. The same applies to programmers and code review.
  • hsn915 5y ago
    Books are published once, not through iterations. The equivalent in book writing would be that you are not allowed to write more than one page at a time, and someone must approve it before you start working on the next page.
    • kqr 5y ago
      Another difference between books and code is that books are read and interpreted by humans (who are very tolerant of grammar bad, in the grand scheme of things) and code is read and interpreted by computers (who will swallow any ambiguity whole and just steam on with their interpretation.) If books were read by machines rather than humans, and were published incrementally, then it would make sense to ask someone to edit each iteration.
    • deleted 5y ago
      [deleted]
• erpellan 5y ago
  That final, 1 word sentence is one of the major problems of PRs. An engineer might spend several hours really thinking through a problem, talking with colleagues, whiteboarding options and coming up with a workable solution that addresses all the obvious issues and a bunch of non-obvious ones. Only for a drive-by take-down by someone with none of the context. It's a totally asymmetric investment of time. Even helpful review comments might only take a minute to write but a day to incorporate.
  • kps 5y ago
    > Only for a drive-by take-down by someone with none of the context. Then write it down. If the code reviewer can't follow what's going on, what hope is there for the new hire looking at it six months from now?
    • ipaddr 5y ago
      Because looking through 6 month old prs are what new hires are doing to learn the codebase?
      • lalaithion 5y ago
        That is one of the things I do to learn new codebases.
      • yxhuvud 5y ago
        Sure. If they are good and have a decent grasp of using version control then will look at the history of a certain piece of code if they don't understand why it is the way it is. I do it often, even when I'm no longer a new hire.
      • detaro 5y ago
        Not necessarily looking through PRs, but presumably they need to work with actual code in your code base that was merged at some point, unless your product is growing so fast that new people just write new code all the time? (Same applies for not-new coworkers that now need to touch that code area anyways)
• AnimalMuppet 5y ago
  It's worse than that. I don't trust me. You shouldn't either. I need the review.
• watwut 5y ago
  The best code I have seen was on projects without code review. And projects with it were more mess - they were surface consistent but overall hard to comprehend. The deciding factor was ownership and accountability tho - you maintained own code and if you done it crappily, you knew. The code review is related to assumption that everyone can change everything - meaning all in all inconsistent mess. It is also related to unfortunate assumption that code review is how people learn system. They don't, unless they are original author. The rest of then knows pieces of it only.
  • devanl 5y ago
    I agree that code review is not a great medium to disseminate information about the codebase, but assigning individual ownership and accountability doesn't make the need to share information go away. Ownership and accountability are workable as long as the same person sticks around long enough to fix all their mistakes. If that person leaves, the code becomes orphaned and it's up to one of their teammates to find out where the skeletons are buried.
    • watwut 5y ago
      No it does not. But it also leads to one-two clear person you know to ask. It also leads to same person explaining same thing multiple times. So as long as he cares just a little, the explanation will improve. And the person doing explanation actually understand the whole part. That is big one too - you are not explained bits of it by someone who knows only small parts of it. And the system was not changed by third party without the person who explains having at least vague idea it was changed. > Ownership and accountability are workable as long as the same person sticks around long enough to fix all their mistakes. If that person leaves, the code becomes orphaned and it's up to one of their teammates to find out where the skeletons are buried. That is actually exactly the same without clear areas. Except everyone is new all the time. If you have high turnover, this aspect will be bad matter what. And it is not that bad with responsibility either. Old person does explains things to new person. Taking over, when you are working consistently in same area is not that hard, actually. Some parts you conclude to be mess, but usually not whole of it. --------- And imo, quality and knowledge heavily depends on other parts of team. Analysis and testing. Both, if work reasonably well are massive resource for knowing how the hell the system is supposed to work. And once you have reliable source for requirements, that is not developers, then deciphering the code is much easier.
• snarf21 5y ago
  I think it is trust in the trust and verify sense. Meaning, we trust each other to do our best but let us verify that we are all on the same page and aren't breaking things or doing something harmful.
  • joombaga 5y ago
    What is the "trust and verify sense"? I've been told this by managers before but I've honestly never understood it. "Trust" to me means "be willing to take action without further examination of the facts at hand". If I trust your code I wouldn't review it. But I don't trust your code, and I don't trust you to have taken all of the appropriate considerations when you were writing it. What's the difference between "trust but verify" and "don't trust, verify"? Is the phrase "trust but verify" an attempt to avoid damaging self-esteem? There seems to be some emotional/ego-based attachment to the concept of trust.
    • strken 5y ago
      I think it's differentiating total lack of trust ("all your PRs suck while mine are perfect, so I have to read yours") from universal fallibility ("your PRs are great, but all code has bugs, so we all review each other's code to catch some of them").
    • jayd16 5y ago
      Its a Reagan quote but in this context it means that you trust their intentions, effort etc, but its still worth checking.
• physicsguy 5y ago
  I require PRs with code review on projects I am an admin on because I don’t trust myself. I sure as hell don’t trust other people!
• marcosdumay 5y ago
  A formal review step where you accept or reject the code, yes, is always there because of lack of trust. Informal reviews can have a lot of different shapes, and provide a lot of different benefits. But anything that is a stop-the-process activity is there because you expect quality problems from the earlier steps. (That said, no good developer trusts oneself. So if you don't have anything on your process that makes you trustworthy, lack of trust is the correct approach.)
• xmlninja 5y ago
  I trust you with my life but do I also trust you with my money and my wife?
• sanderjd 5y ago
  Yes, catching mistakes (which everyone makes) and bidirectional communication are the points of code reviews, it's not a lack of trust.
• globalise83 5y ago
  Trust but verify
• actually_a_dog 5y ago
  Absolutely. I take issue with all 3 of his main points. You covered the first point well, so, I'll take the last 2. > Pull requests don't prevent bugs. Reviewing code is hard! You can catch obvious mistakes, but you won't have consistent in-depth scrutiny. Many issues reveal themselves only after extensive use, which doesn't happen during code reviews. We rely on fast iterations with user feedback. So many things I can say about this. "Fast iterations with user feedback" are great, until you introduce a security bug that you find out about because someone exploits it. Catching "obvious" mistakes alone has significant value. Since he acknowledges that code reviews catch some bugs, this criticism seems to be that they don't catch all bugs. To that, I say: show me a practice which does catch all bugs, and I'll show you why it doesn't. Even the most rigorous development process doesn't prevent all bugs. [0] This just doesn't sound like a fair criticism to me. > 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. This is largely a culture problem. Code review should be a high priority for engineers. Maybe it's not the highest priority, but, it's fair to expect one could get a decent review on a PR that isn't too large in, say, 1-2 days or less. If you have PRs sitting in review for weeks at a time on a regular basis[1], one of two things is happening: either engineers aren't properly prioritizing code review, or your PRs are too large and/or have too many dependencies. Which brings me to the second thing you can do, which is make PRs that are as small and simple as possible, but no more. Minimize dependencies to help make your PRs understandable. And, it should be fairly obvious, but the fewer lines of code (including config changes) you have in your PR, the easier it will be to review, so, the faster it will get done. Code review is also a great opportunity to learn and teach. Every senior+ engineer should be reviewing code frequently, IMO. Finally, yes, code reviews will slow you down if your basis for comparison is just "engineers merge code when they think it's ready." That's intentional, and good. The cost is more than paid for by better code quality, leading to fewer instances of "here's a PR to fix the issues with my previous PR." --- [0]: https://news.ycombinator.com/item?id=29801451 https://news.ycombinator.com/item?id=29801451 [1]: I've only had this happen once or twice in my career so far. In each case, it was because the code surrounding the code I was working on was not stable and kept changing. These were clearly exceptional instances.
• mulmen 5y ago
  I tend to scan most reviews for the team even if the code review itself is not directed to me (we use a round-robin/domain expert routing method). It helps me understand what my teammates are working on and give me a better understanding of our system. I can't imagine anyone interpreting that as a lack of trust. Is looking at our source code inherently distrustful?
• jayd16 5y ago
  If it wasn't about trust you could just read the commit history. Its certainly about trust.
• asiachick 5y ago
  Agreed, so many times reviewers have saved me from myself as well as passed on solutions I didn't know, pointed out functions in our giant codebase I wasn't aware of, taught me new language concepts I wasn't familiar/comfortable with (ie, C++11->14->17->20) and generally helped limit our technical debt. I'm a fan of code review For me, code review is collaboration. We're helping each other. The one thing I'd suggest is, as much as possible, try to phrase reviews like a question: "Do you mean to do X here?", "Would using fooTheFob here work?", "Is this a race condition?". At least it feels better to me.
• mpweiher 5y ago
  > Is that why we do code reviews, because we don't trust each other? I don't see any other reason to require code reviews before a change can be merged. > humans make mistakes Then write unit tests, religiously. And pair. Or "... request reviews when they think it's necessary". In my experience code reviews are not very good at catching mistakes. > Different work in progress can be in conflict with each other. Code reviews are the cause of too much work in progress, not the solution. Not the only cause. > Reviews are a good way to learn from each other In my experience, they are pretty bad at this. Pairing is vastly superior, or giving talks. > reason to communicate with devs I might not otherwise communicate with Then communicate with them. And if you want a review "... request reviews when they think it's necessary". > Rejected Exactly. That's not a useful form of interacting with your peers.
  • nrb 5y ago
    > I don't see any other reason to require code reviews before a change can be merged. In the US, a SOC 2 audit of your org’s change management process is going to be a really bad time without this.
    • mpweiher 5y ago
      Do I really have to add "...unless required by law/contract"?
      • nrb 5y ago
        Well, doing this _before_ undergoing these certifications would still be a good reason that was omitted from the very absolute claim that was made.
• damethos 5y ago
  I completely agree. This article is bad advice, please do not follow it.