5 ms·

Unpopular (?) Opinion: In most "modern" environments, with constant online updates, where shipped defects are not that costly anymore, code review is a net los

Unpopular (?) Opinion: In most "modern" environments, with constant online updates, where shipped defects are not that costly anymore, code review is a net loss. You can just skip it. This study fails to compare with "no code review whatsoever" and it measures by "defects found" instead of "total effort spent" (i.e. the only metric that truly matters). In code review, most developers will not do the kind of thorough analysis that uncovers serious bugs, if they actually did, code review would be too costly. Instead, disagreements on superficialities and bike-shed arguments are promoted, often leading to low-value follow-up changes that can easily cause worse issues than they solve. Code review satisfies the urge for process and structure, but it also satisfies the need for diffusion of responsibility, which is a bad thing. People need to own their code. Code reviews have some value during onboarding or with junior devs, to get everybody on the same page. After that, you can disregard it, the effort is better spent on other things like automated testing.

badlucklottery 8y ago
>In code review, most developers will not do the kind of thorough analysis that uncovers serious bugs, if they actually did, code review would be too costly. The point isn't to go in-depth pick apart the design (if a dev needs that kind of deep intervention on a regular basis, it's time to have some hard conversations), it's to share knowledge. The reviewer gets to see a another dev's code along with a good description of what that change does so if there's an emergency someone on the team might actually know where to look. And the submitter gets to know of any footguns the reviewer has seen in similar code. >Instead, disagreements on superficialities and bike-shed arguments are promoted, often leading to low-value follow-up changes that can easily cause worse issues than they solve. Which is why it's important to have a system where the submitter can choose the reviewer(s) to avoid bike-shedding and also choose to ignore the reviewers' comments. That's Google's system. Is someone throwing a lot of BS nits at you? Ack the comments, take them off the reviewers list and pick someone else. Do that a couple times and most folks get the message. Talk to management about those that don't.
- beefheart 8y ago
  > The reviewer gets to see a another dev's code along with a good description of what that change does so if there's an emergency someone on the team might actually know where to look. In an emergency, you need developers to be able to figure things out regardless of whether they have seen something in a code review or not. Knowledge of a code base is built much more efficiently by actually working on it and, to a lesser degree, reading the code. With the time you save not doing reviews, you can let people write tests or do refactorings to build that exact skill directly, instead of having it be a side-effect.
  - bcyn 8y ago
    You may be underestimating how effective this type of knowledge sharing is. Knowing where to look is half the battle.
falsedan 8y ago
In my experience, code reviews 1. Check that the code is addressing the problem at hand and is fulfilling a business need (mostly making sure it’s linked to a reasonably-written ticket) 2. Align style and make code look consistent across repos/teams (which includes pointing out similar work which could be reused) 3. Force other people to read and understand the change, so the original author isn’t on the hook for all support/oncall questions) 4. Highlight hard-to-understand sections that might be unclear to someone newly-arriving to the code, suggesting those parts need more explanation or a simpler implementation 5. Copy editing on error messages and docs to remove typos/passive-aggressiveness/make important information easier to see 6. Find bugs by humans running static analysis (“this will race/deadlock”) and provide pointers to resources to help the reviewee learn (“see <some link> for why and how to avoid it”) 7. Human-run linting (if the automatic linters miss/mess up stuff) I’ve also see it used as a way to bully other less-knowledgeable devs by nitpicking and issue-raising when the commenter isn’t included on the original list of reviewers and leaves criticism without any constructive feedback (“this is wrong”) Rarely have we spotted bugs in code review: that’s what tests, oncall, and a robust rollback mechanism are for
- dasmoth 8y ago
  mostly making sure it’s linked to a reasonably-written ticket This one seems to come up quite often recently, but to me sounds an awful lot like "satisfying the urge for process and structure". Yes, code people write in professional environments should generally be satisfying user/business requirements, but that's something that can be discussed periodically, perhaps with your manager -- it doesn't seem like something that needs somebody looking over your shoulder all the time. Ticketing systems can be useful tools, especially in environments with lots of user-submitted requests, but I'm suspicious of trying to tie every single piece of work to a ticket. That seems rather like a tool which should be your servant becoming master. Edit: if you can't have a sensible conversation, potentially with a not-especially-technical person, about what value you've created in the past few months, then you might have a problem. One of my worries about ticket-focussed environments is that it's possible to focus on the small-scale and lose sight of what you're creating.
  - 8y ago
sheepmullet 8y ago
> In code review, most developers will not do the kind of thorough analysis that uncovers serious bugs, if they actually did, code review would be too costly We replaced code reviews of every commit with in-depth code walkthroughs. Basically whenever we start having a lot of issues with a subsystem we organise up to five half day sessions to go through the entire subsystem line by line as a group. It brings team progress on other areas to a standstill while we do it but works way better than normal code reviews.
tigershark 8y ago
I completely disagree, if I could have a penny for a bug that I found in a code review I’ll be rich.
- Too 8y ago
  Completely disagree also. Around 50% of all patches I review have real logic bugs in them. Patches I write myself aren't flawless either and often someone finds bugs in them as well. And it's not like you have to spend several hours on thorough analysis either. Small changes takes a few minutes, medium changes 30min and larger changes should deserve a few hours. If your team is bikeshedding in code reviews, install an automatic code formatter.
a_imho 8y ago
Personally I'm at the other end of things, I routinely ask people to look at my code multiple times even before submitting it for code review. Chances I could learn about some edge case I did not consider before.

badlucklottery 8y ago

>In code review, most developers will not do the kind of thorough analysis that uncovers serious bugs, if they actually did, code review would be too costly. The point isn't to go in-depth pick apart the design (if a dev needs that kind of deep intervention on a regular basis, it's time to have some hard conversations), it's to share knowledge. The reviewer gets to see a another dev's code along with a good description of what that change does so if there's an emergency someone on the team might actually know where to look. And the submitter gets to know of any footguns the reviewer has seen in similar code. >Instead, disagreements on superficialities and bike-shed arguments are promoted, often leading to low-value follow-up changes that can easily cause worse issues than they solve. Which is why it's important to have a system where the submitter can choose the reviewer(s) to avoid bike-shedding and also choose to ignore the reviewers' comments. That's Google's system. Is someone throwing a lot of BS nits at you? Ack the comments, take them off the reviewers list and pick someone else. Do that a couple times and most folks get the message. Talk to management about those that don't.

beefheart 8y ago
> The reviewer gets to see a another dev's code along with a good description of what that change does so if there's an emergency someone on the team might actually know where to look. In an emergency, you need developers to be able to figure things out regardless of whether they have seen something in a code review or not. Knowledge of a code base is built much more efficiently by actually working on it and, to a lesser degree, reading the code. With the time you save not doing reviews, you can let people write tests or do refactorings to build that exact skill directly, instead of having it be a side-effect.
- bcyn 8y ago
  You may be underestimating how effective this type of knowledge sharing is. Knowing where to look is half the battle.

falsedan 8y ago

In my experience, code reviews 1. Check that the code is addressing the problem at hand and is fulfilling a business need (mostly making sure it’s linked to a reasonably-written ticket) 2. Align style and make code look consistent across repos/teams (which includes pointing out similar work which could be reused) 3. Force other people to read and understand the change, so the original author isn’t on the hook for all support/oncall questions) 4. Highlight hard-to-understand sections that might be unclear to someone newly-arriving to the code, suggesting those parts need more explanation or a simpler implementation 5. Copy editing on error messages and docs to remove typos/passive-aggressiveness/make important information easier to see 6. Find bugs by humans running static analysis (“this will race/deadlock”) and provide pointers to resources to help the reviewee learn (“see <some link> for why and how to avoid it”) 7. Human-run linting (if the automatic linters miss/mess up stuff) I’ve also see it used as a way to bully other less-knowledgeable devs by nitpicking and issue-raising when the commenter isn’t included on the original list of reviewers and leaves criticism without any constructive feedback (“this is wrong”) Rarely have we spotted bugs in code review: that’s what tests, oncall, and a robust rollback mechanism are for

dasmoth 8y ago
mostly making sure it’s linked to a reasonably-written ticket This one seems to come up quite often recently, but to me sounds an awful lot like "satisfying the urge for process and structure". Yes, code people write in professional environments should generally be satisfying user/business requirements, but that's something that can be discussed periodically, perhaps with your manager -- it doesn't seem like something that needs somebody looking over your shoulder all the time. Ticketing systems can be useful tools, especially in environments with lots of user-submitted requests, but I'm suspicious of trying to tie every single piece of work to a ticket. That seems rather like a tool which should be your servant becoming master. Edit: if you can't have a sensible conversation, potentially with a not-especially-technical person, about what value you've created in the past few months, then you might have a problem. One of my worries about ticket-focussed environments is that it's possible to focus on the small-scale and lose sight of what you're creating.
- 8y ago

sheepmullet 8y ago

> In code review, most developers will not do the kind of thorough analysis that uncovers serious bugs, if they actually did, code review would be too costly We replaced code reviews of every commit with in-depth code walkthroughs. Basically whenever we start having a lot of issues with a subsystem we organise up to five half day sessions to go through the entire subsystem line by line as a group. It brings team progress on other areas to a standstill while we do it but works way better than normal code reviews.

tigershark 8y ago

I completely disagree, if I could have a penny for a bug that I found in a code review I’ll be rich.

Too 8y ago
Completely disagree also. Around 50% of all patches I review have real logic bugs in them. Patches I write myself aren't flawless either and often someone finds bugs in them as well. And it's not like you have to spend several hours on thorough analysis either. Small changes takes a few minutes, medium changes 30min and larger changes should deserve a few hours. If your team is bikeshedding in code reviews, install an automatic code formatter.

a_imho 8y ago

Personally I'm at the other end of things, I routinely ask people to look at my code multiple times even before submitting it for code review. Chances I could learn about some edge case I did not consider before.