5 ms·

I'm currently at a place with a culture of thorough code reviews. IMO, it entirely depends on the people and the culture. It's on the reviewer to actually thoro

by ufmace 6y ago

I'm currently at a place with a culture of thorough code reviews. IMO, it entirely depends on the people and the culture. It's on the reviewer to actually thoroughly review the code and ensure it does what it's supposed to do, including local checkout and testing if needed. And of course on the management structure to provide senior enough reviewers with enough time to do that. It certainly is easier and less time-consuming to just skim it and comment on some style nitpicks and unit tests, but there's no reason it has to be. The flip side of course is that the original coder does still need to be reasonably competent. Relying on even exhaustively thorough and repeated code reviews to turn basically garbage into reasonable quality code is a bad bet.

legerdemain 6y ago
At the moderately large, very successful companies where I've worked, code is mostly disposable. It's written and read, on average, once. Code craftsmanship doesn't make any sense in these environments. We treat code review as a formality. Like, we just kind of skim the diff and click the approve button, but most of the time none of us really have the time to understand the code on a deep level. Correctness is one competent dev banging on the code until it works, and then getting git-blames and bug reports sent directly to them when it stops working. We don't blame the reviewer for our own bugs. If the dev leaves, it's faster for the next dev to paper over the bug or rewrite the feature than it is to do a close reading of the existing codebase. That's what "rapid iteration" and "agile development" is all about.
BobbyJo 6y ago
I am vehemently against making reviewers checkout and test code. You should have tests that demonstrate correctness with very high confidence, and a release process that can find bugs early. Expecting reviewers to checkout and run/test things is such a time sink, and over the course of a year of doing it at a new work place, I've never seen it be the thing that caught a bug. If anything I feel like it caused more bugs because people end up getting laxed with tests.
- JKCalhoun 6y ago
  It's as though code reviews are a waste of time. I have to say, now over 30 years as a developer, I've only been introduced to code reviews in the last few years. Somehow, I have no idea how, we shipped code for nearly 30 years without code reviews or unit tests. Thank god we now no longer ship with bugs.
  - doctor_eval 6y ago
    Although I think you’re being sarcastic, I’m sort of in the same camp. We started doing code reviews a few years ago after years of informal releases, and I found they acted like a sieve; I reckon we halved our deployed bug count, at reasonably low cost, but also code style and knowledge improved across the board. Code reviews are not a silver bullet, and we still released some whoppers, but halving the deployed bug count and improving the code base is a worthwhile outcome. We’ve recently started doing CD with E2E tests and I reckon E2E has halved the deployed bug count again. It’s definitely stopped bugs from entering production. There always seems to be a trade off between the cost of action (in resources, time to release, flexibility) and quality, there is no right answer (despite what people will tell you), and it’s up to you to set the acceptable bar for released bugs, and to deal with the consequences. For me, code reviews improved bug counts less than I’d expected but had other quality benefits that I felt more than made up for the modest cost of the additional ceremony.
    - JKCalhoun 6y ago
      I think you have a point about spreading knowledge. Rather than code review though, I think having engineers pair up on a feature is another (better?)way to knowledge share that probably is more efficient time-wise?
      - lmilcin 6y ago
        I prefer pairing up. Giving feedback on code that has already been written, and by more junior member of the team, is fraught with problems. People get defensive thinking they have already completed the task and now I am delaying the closure. It is much easier to share friendly hints and observations while writing the code. When the code is written it is difficult to show how it could have looked or demonstrate the process of getting there. And so even though my org requires us to conduct code reviews (and I find them helpful, at a hefty cost), I will still ask people to pair up with me regularly. This to get to know the person better, to see what the problem is (and sometimes I am the problem), to help work out better design, etc. All of which is difficult with regular review.

by ufmace 6y ago

legerdemain 6y ago

At the moderately large, very successful companies where I've worked, code is mostly disposable. It's written and read, on average, once. Code craftsmanship doesn't make any sense in these environments. We treat code review as a formality. Like, we just kind of skim the diff and click the approve button, but most of the time none of us really have the time to understand the code on a deep level. Correctness is one competent dev banging on the code until it works, and then getting git-blames and bug reports sent directly to them when it stops working. We don't blame the reviewer for our own bugs. If the dev leaves, it's faster for the next dev to paper over the bug or rewrite the feature than it is to do a close reading of the existing codebase. That's what "rapid iteration" and "agile development" is all about.

BobbyJo 6y ago

I am vehemently against making reviewers checkout and test code. You should have tests that demonstrate correctness with very high confidence, and a release process that can find bugs early. Expecting reviewers to checkout and run/test things is such a time sink, and over the course of a year of doing it at a new work place, I've never seen it be the thing that caught a bug. If anything I feel like it caused more bugs because people end up getting laxed with tests.

JKCalhoun 6y ago
It's as though code reviews are a waste of time. I have to say, now over 30 years as a developer, I've only been introduced to code reviews in the last few years. Somehow, I have no idea how, we shipped code for nearly 30 years without code reviews or unit tests. Thank god we now no longer ship with bugs.
- doctor_eval 6y ago
  Although I think you’re being sarcastic, I’m sort of in the same camp. We started doing code reviews a few years ago after years of informal releases, and I found they acted like a sieve; I reckon we halved our deployed bug count, at reasonably low cost, but also code style and knowledge improved across the board. Code reviews are not a silver bullet, and we still released some whoppers, but halving the deployed bug count and improving the code base is a worthwhile outcome. We’ve recently started doing CD with E2E tests and I reckon E2E has halved the deployed bug count again. It’s definitely stopped bugs from entering production. There always seems to be a trade off between the cost of action (in resources, time to release, flexibility) and quality, there is no right answer (despite what people will tell you), and it’s up to you to set the acceptable bar for released bugs, and to deal with the consequences. For me, code reviews improved bug counts less than I’d expected but had other quality benefits that I felt more than made up for the modest cost of the additional ceremony.
  - JKCalhoun 6y ago
    I think you have a point about spreading knowledge. Rather than code review though, I think having engineers pair up on a feature is another (better?)way to knowledge share that probably is more efficient time-wise?
    - lmilcin 6y ago
      I prefer pairing up. Giving feedback on code that has already been written, and by more junior member of the team, is fraught with problems. People get defensive thinking they have already completed the task and now I am delaying the closure. It is much easier to share friendly hints and observations while writing the code. When the code is written it is difficult to show how it could have looked or demonstrate the process of getting there. And so even though my org requires us to conduct code reviews (and I find them helpful, at a hefty cost), I will still ask people to pair up with me regularly. This to get to know the person better, to see what the problem is (and sometimes I am the problem), to help work out better design, etc. All of which is difficult with regular review.