3 ms·

The problem with code review is that it works. That's why we can't shake the industry's fascination with it. It works to catch bugs, but it's horrendously ineff

The problem with code review is that it works. That's why we can't shake the industry's fascination with it. It works to catch bugs, but it's horrendously inefficient in doing so. I ran some numbers on our code review process on a project last year. Of patches returned for modification, 5% contained a bug. 95% were for entirely stylistic changes. I find it hard to square that with being a good use of time. As an industry, we're making the mistake Deming points out in Out Of The Crisis: we're attempting to inspect defects out of existence rather than improve our production process so that they never get created in the first place. We concentrate our efforts on being better at 100% inspection, and don't allow ourselves to think that a process without that inspection could be a hell of a lot better, if we invested the effort in figuring out what that might look like. Pairing might be one answer here, although I don't think I've heard it talked about in those terms before.

Sharlin 8y ago
Code review was never about catching bugs. Not primarily, anyway. Code is written for humans to read (and modify), and a reviewer's job is to make sure the code can adequately fulfill that purpose. A reviewer is a proxy for the person who has to touch the code in the future – indeed, often they're the same person! Lumping all code quality issues under "stylistic changes" is just a disingenuous false dichotomy.
- regularfry 8y ago
  > Code review was never about catching bugs. Not primarily, anyway. That's incorrect. Code review originated as a technique specifically to find defects, under the name Fagan Inspection. That we've found peripheral benefits is great, but the central justification is defect reduction. > Code is written for humans to read (and modify), and a reviewer's job is to make sure the code can adequately fulfill that purpose. I quite like that formulation, but it doesn't undercut the point that we only seem to be able to cope by having a second (third, fourth) pair of eyeballs inspecting every single line of code after it's been written, rather than working on systems to get it right first time. > false dichotomy It's not, though. There is a real difference. If you find a defect in the code, you've definitely prevented a bug from reaching production today. If you find a style problem in the code, you've maybe prevented next month's work from slowing down. If the project stopped tomorrow, none of the style issues would matter, and the code would continue working just fine.
petetnt 8y ago
First step to get right would be having automatic code formatters in place properly to cut out stylistic nits. With tools like Prettier, gofmt, yapf, Black and similar you can focus on the code itself, not how it looks. Of course it can take a while to find some common ground to some stylistic options (which is why I love opinionated formatters that don't give the choice) but it pays off quickly regardless and you gain huge wins with a tiny set of tooling. For myself, I actually stopped caring of code styles and only run code formatters in a pre-commit step (if possible). By giving all the trust of fixing stylistic issues to the formatter and just focusing on the code itself while coding I ensure myself that I focus on the most important thing: getting stuff done.
- regularfry 8y ago
  That takes care of the lowest-level stylistic issues, but doesn't capture things like "why are we trapping errors here rather than in a central error handler there". The sort of question where the app will function either way, but reasonable people can have aesthetic disagreements, and a formatter is totally useless.
edmundhuber 8y ago
I agree that most code review change requests are style-related. One reason I can think of this happening is that in order for one's brain to focus on just the parts of the CR that matter, stylistic oddities need to be sorted out. Ideally after some time, a reviewee would stop opening PRs that contradict the basic stylistic standards that the group adheres to (and thus stop wasting everyone's time). Ideally there would also be a linter to get everyone to that point as quickly as possible. Imagine if you were running an assembly line, and every person in every shift did their welds significantly differently. It would be a waste of time for the inspection workers to have to continuously remind everyone that it's hard to check quality on a variety of different welding methods, rather than just one or two. It would be a better use of everyone's time to just agree on what kinds of welds are acceptable.
drej 8y ago
Fascinating numbers. Do you know if that 95% number could be brought down by tooling? We have deployed fairly strict code style guides + included some linters and other tools to watch over code quality to reduce these conversations about style, but it obviously doesn't catch everything.
- regularfry 8y ago
  That's with style tools in place.
goto11 8y ago
What kind of stylistic changes are you talking about? If it is things like whitespace and brace placement style - yeah you are wasting your time. Decide on a style and have a tool enforce it. But code reviews are for other things than just catching bugs, it is also for sharing knowledge, learning useful idioms for each other, discussing ideas and so on. I have learnt a lot from code reviews, even when the reviewers were junior to me. Feedback like "there is actually already a function for this" or "this can be done simpler if you use X language feature" have great value even if it is not directly catching a bug. And why is it a bad use of time if it actually helps finding bugs? What method do you know that is more efficient?
- regularfry 8y ago
  > What kind of stylistic changes are you talking about? It's the level above the decisions a tool can enforce which I've seen tending towards bikeshedding. > But code reviews are for other things than just catching bugs... I don't dispute that there is value here. I think that value is tremendously overplayed in comparison to the costs incurred. Or rather, I think the costs are underplayed. > And why is it a bad use of time if it actually helps finding bugs? Tweak the numbers slightly: would it be reasonable, for that purpose, at a rate of 1%? 0.1%? Think how much time has been spent on the reviews at that point, and how much delay has been injected between initial commit and release. There comes a point where you have to look at the process and think "this is not a worthwhile investment of everyone's time." > What method do you know that is more efficient? I don't. That's the point. As an industry we seem to have latched on to code reviews because they're one of the very few techniques which there's actually any evidence for, to the extent that questioning whether they're the best we can do is discouraged.
  - goto11 8y ago
    Code reviews should not devolve into bikeshedding. That is a problem of people not understanding the purpose and value of code reviews, not a problem with the concept of code reviews. Ignore the issue if it is not important or make a ruling and move on. If your code reviews find bugs left and right I would say it indicates a deeper problem: Why are there so many easy-to-spot bugs in the first place? The amount of bugs found is definitely not a measurement of success. The purpose of review is to improve overall readability, competence, and code quality to level were fewer bugs are introduced in the first place.
mycelium 8y ago
The inefficiency you speak of is not inefficiency of code review, but rather the inefficiency of reaching a shared, deep agreement on what your code should look like and how it should work. Code review is just a place that it pops up. Thinking of it as inspection alone is a disservice to the cultural value of code review. You want a process that can teach a team member to contribute with lesser inspection? Code review. I've typically found that with a new team or team member, initially there are a lot of patches returned for modification. After 3-9 months, it tends towards 80% of patches being one-shot LGTMs, the remaining 20% having spec issues or substantive style issues (ie. this module structure will bite us in the butt because...) This idea of communicating shared knowledge also points at more efficient ways to do that, if CR is a bottleneck: - Technical onboarding - Google does a great job of this. Taking time to explain how to work with your technologies, and what the expected code style is. - Linting + style guides - Arguing over style is dumb.
- regularfry 8y ago
  This is great as long as you've got a culture which tends towards trusting LGTM's. That's not inevitable; you can just as easily get to a point of escalating nit-pickery. However, even if you get to a point where most PR's get waved through, it's still inefficient in the sense that finished code ends up sitting around in a queue, waiting for its LGTMs before it can go to production. That can add days of delay, for no added value in the majority of cases.
Double_a_92 8y ago
Code reviews are not only there to find bugs. They can "just" help to improve code readability and extendability, so less bugs are produced in the future and it's easier to develop new features.
jyounker 8y ago
The view of pairing as a form of continuous code review is not a new insight, but it's also not a widely recognized or appreciated one.

Sharlin 8y ago

Code review was never about catching bugs. Not primarily, anyway. Code is written for humans to read (and modify), and a reviewer's job is to make sure the code can adequately fulfill that purpose. A reviewer is a proxy for the person who has to touch the code in the future – indeed, often they're the same person! Lumping all code quality issues under "stylistic changes" is just a disingenuous false dichotomy.

regularfry 8y ago
> Code review was never about catching bugs. Not primarily, anyway. That's incorrect. Code review originated as a technique specifically to find defects, under the name Fagan Inspection. That we've found peripheral benefits is great, but the central justification is defect reduction. > Code is written for humans to read (and modify), and a reviewer's job is to make sure the code can adequately fulfill that purpose. I quite like that formulation, but it doesn't undercut the point that we only seem to be able to cope by having a second (third, fourth) pair of eyeballs inspecting every single line of code after it's been written, rather than working on systems to get it right first time. > false dichotomy It's not, though. There is a real difference. If you find a defect in the code, you've definitely prevented a bug from reaching production today. If you find a style problem in the code, you've maybe prevented next month's work from slowing down. If the project stopped tomorrow, none of the style issues would matter, and the code would continue working just fine.

petetnt 8y ago

First step to get right would be having automatic code formatters in place properly to cut out stylistic nits. With tools like Prettier, gofmt, yapf, Black and similar you can focus on the code itself, not how it looks. Of course it can take a while to find some common ground to some stylistic options (which is why I love opinionated formatters that don't give the choice) but it pays off quickly regardless and you gain huge wins with a tiny set of tooling. For myself, I actually stopped caring of code styles and only run code formatters in a pre-commit step (if possible). By giving all the trust of fixing stylistic issues to the formatter and just focusing on the code itself while coding I ensure myself that I focus on the most important thing: getting stuff done.

regularfry 8y ago
That takes care of the lowest-level stylistic issues, but doesn't capture things like "why are we trapping errors here rather than in a central error handler there". The sort of question where the app will function either way, but reasonable people can have aesthetic disagreements, and a formatter is totally useless.

edmundhuber 8y ago

I agree that most code review change requests are style-related. One reason I can think of this happening is that in order for one's brain to focus on just the parts of the CR that matter, stylistic oddities need to be sorted out. Ideally after some time, a reviewee would stop opening PRs that contradict the basic stylistic standards that the group adheres to (and thus stop wasting everyone's time). Ideally there would also be a linter to get everyone to that point as quickly as possible. Imagine if you were running an assembly line, and every person in every shift did their welds significantly differently. It would be a waste of time for the inspection workers to have to continuously remind everyone that it's hard to check quality on a variety of different welding methods, rather than just one or two. It would be a better use of everyone's time to just agree on what kinds of welds are acceptable.

drej 8y ago

Fascinating numbers. Do you know if that 95% number could be brought down by tooling? We have deployed fairly strict code style guides + included some linters and other tools to watch over code quality to reduce these conversations about style, but it obviously doesn't catch everything.

regularfry 8y ago
That's with style tools in place.

goto11 8y ago

What kind of stylistic changes are you talking about? If it is things like whitespace and brace placement style - yeah you are wasting your time. Decide on a style and have a tool enforce it. But code reviews are for other things than just catching bugs, it is also for sharing knowledge, learning useful idioms for each other, discussing ideas and so on. I have learnt a lot from code reviews, even when the reviewers were junior to me. Feedback like "there is actually already a function for this" or "this can be done simpler if you use X language feature" have great value even if it is not directly catching a bug. And why is it a bad use of time if it actually helps finding bugs? What method do you know that is more efficient?

regularfry 8y ago
> What kind of stylistic changes are you talking about? It's the level above the decisions a tool can enforce which I've seen tending towards bikeshedding. > But code reviews are for other things than just catching bugs... I don't dispute that there is value here. I think that value is tremendously overplayed in comparison to the costs incurred. Or rather, I think the costs are underplayed. > And why is it a bad use of time if it actually helps finding bugs? Tweak the numbers slightly: would it be reasonable, for that purpose, at a rate of 1%? 0.1%? Think how much time has been spent on the reviews at that point, and how much delay has been injected between initial commit and release. There comes a point where you have to look at the process and think "this is not a worthwhile investment of everyone's time." > What method do you know that is more efficient? I don't. That's the point. As an industry we seem to have latched on to code reviews because they're one of the very few techniques which there's actually any evidence for, to the extent that questioning whether they're the best we can do is discouraged.
- goto11 8y ago
  Code reviews should not devolve into bikeshedding. That is a problem of people not understanding the purpose and value of code reviews, not a problem with the concept of code reviews. Ignore the issue if it is not important or make a ruling and move on. If your code reviews find bugs left and right I would say it indicates a deeper problem: Why are there so many easy-to-spot bugs in the first place? The amount of bugs found is definitely not a measurement of success. The purpose of review is to improve overall readability, competence, and code quality to level were fewer bugs are introduced in the first place.

mycelium 8y ago

The inefficiency you speak of is not inefficiency of code review, but rather the inefficiency of reaching a shared, deep agreement on what your code should look like and how it should work. Code review is just a place that it pops up. Thinking of it as inspection alone is a disservice to the cultural value of code review. You want a process that can teach a team member to contribute with lesser inspection? Code review. I've typically found that with a new team or team member, initially there are a lot of patches returned for modification. After 3-9 months, it tends towards 80% of patches being one-shot LGTMs, the remaining 20% having spec issues or substantive style issues (ie. this module structure will bite us in the butt because...) This idea of communicating shared knowledge also points at more efficient ways to do that, if CR is a bottleneck: - Technical onboarding - Google does a great job of this. Taking time to explain how to work with your technologies, and what the expected code style is. - Linting + style guides - Arguing over style is dumb.

regularfry 8y ago
This is great as long as you've got a culture which tends towards trusting LGTM's. That's not inevitable; you can just as easily get to a point of escalating nit-pickery. However, even if you get to a point where most PR's get waved through, it's still inefficient in the sense that finished code ends up sitting around in a queue, waiting for its LGTMs before it can go to production. That can add days of delay, for no added value in the majority of cases.

Double_a_92 8y ago

Code reviews are not only there to find bugs. They can "just" help to improve code readability and extendability, so less bugs are produced in the future and it's easier to develop new features.

jyounker 8y ago

The view of pairing as a form of continuous code review is not a new insight, but it's also not a widely recognized or appreciated one.