Confessions of a programmer: I hate code review (2010) • Hacker News Board

Re: Confessions of a programmer: I hate code review (2010)

#141

Post by regularfry » Mon, Oct 29, 2018, 5:47 PM UTC

Sharlin wrote: post #27

regularfry wrote: post #17

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 indu…

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.

> 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.

Re: Confessions of a programmer: I hate code review (2010)

#142

Post by dvlsg » Mon, Oct 29, 2018, 5:52 PM UTC

kochthesecond wrote: post #85

falsedan wrote: post #32

Earlier quoted context omitted.

Great points. Regarding point 2: > 2. The reviewer concedes that there can be equally valid approaches to a given problem or taste wrt. aesthetics. They do not try to gratuitously force their style upon the reviewee. The reciprocal must hold as well. If my code is being reviewed, and the reviewer has opinions about how they would do it, I like it when they share their preference but also 1. consider if the code as su…

A few comments. Very rarely I get stuff that is so badly made, or naively implemented that I just have to say no, then go help the person redesign the solution. This happens rarely though, maybe twice a year with a fresh employee. If the code is not formatted to the linter, it should not have been submitted for review and I will probably reject it. We have to hold our work to some kind of standard. Otherwise I do the…

> If the code is not formatted to the linter, it should not have been submitted for review and I will probably reject it.

Better yet, make a tool (CI) automatically reject it. That way it doesn't waste your time, and no one has to be the "bad guy".

Not until developers start disabling the linter with inline comments, anyways.

Re: Confessions of a programmer: I hate code review (2010)

#143

Post by eridius » Mon, Oct 29, 2018, 6:03 PM UTC

Eleopteryx wrote: post #127

eridius wrote: post #9

You know what I hate more than code review? Shipping bugs. The author of this piece admits that they have to change the code they write in response to comments. This means the code review is flagging areas of improvement. Surely the resulting code is better than the original code (otherwise I'd expect the author to push back on the comments instead of implementing bad suggestion). Similarly, when acting as the code r…

To me, code review is a combination of two things I would think we would try to avoid: 1. Testing implementation over behavior 2. Using a human being to test in a non-automated way Proper test coverage and human QA to me is the most effective approach to not shipping bugs, because it really doesn't matter to the customer what the code does or doesn't look like if it runs. And I just don't believe that code review is…

There are so many bugs that I've caught in code review that would not have been caught by automated or manual testing. Two big reasons for that:

1. Automated testing is still written by a human. A human who doesn't understand the edge cases in their code isn't going to understand how to write tests for those edge cases either. And "have someone else write the tests" won't work either, because frequently these edge cases are a result of the code implementation, not inherent in the problem, and so without a thorough understanding of the code you wouldn't even know the edge cases is there to be tested. Same goes for manual testing too, the manual testcases are still constructed by a human.

2. Race conditions, or rare conditions that most people won't hit. These are rather unlikely to be uncovered by testing (whether automated or manual). Race conditions in general are hard to test, and rare conditions are, well, rare.

There's also just cases like unexpected interaction paths that can produce improperly-handled states in the code, or bugs that don't result in an obvious bug to the observer but are still incorrect (the obvious example here is memory leaks, but this could also just be leaving the app in a bad state such that subsequent actions would fail, but you don't catch those because your test is done at this point).

Re: Confessions of a programmer: I hate code review (2010)

#144

Post by regularfry » Mon, Oct 29, 2018, 6:18 PM UTC

mycelium wrote: post #42

regularfry wrote: post #17

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 indu…

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 revie…

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.

Re: Confessions of a programmer: I hate code review (2010)

#145

Post by regularfry » Mon, Oct 29, 2018, 6:27 PM UTC

goto11 wrote: post #41

regularfry wrote: post #17

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 indu…

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 w…

> 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.

Re: Confessions of a programmer: I hate code review (2010)

#146

Post by regularfry » Mon, Oct 29, 2018, 6:27 PM UTC

drej wrote: post #36

regularfry wrote: post #17

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 indu…

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.

That's with style tools in place.

Re: Confessions of a programmer: I hate code review (2010)

#147

Post by pm24601 » Mon, Oct 29, 2018, 6:45 PM UTC

bryanrasmussen wrote: post #72

RandomInteger4 wrote: post #58

Earlier quoted context omitted.

My CSS-foo is fairly weak; what problems would em cause such that they disallow it in the code base?

em is relative to the font size of the nearest parent thus you can have situations where your font size of an element node is radically different due to the parent it is placed in. This can of course be powerful if used correctly, but it takes someone really good to use it correctly and perhaps it shouldn't be done in big teams because big teams imply some people not very good in some particular discipline. on edit:…

Thanks! I never understood this difference between em, rem, px.

I learned something very new and I have been doing web development (backend mostly) for a long time.

Re: Confessions of a programmer: I hate code review (2010)

#148

Post by koala_man » Mon, Oct 29, 2018, 7:48 PM UTC

vfc1 wrote: post #108

koala_man wrote: post #16

The arguments essentially boil down to: * "I don't have time to review people's code, I just want to code" * "I don't have time to have my code reviewed, because if it's bad I'd have to fix it" This really mirrors people's complaints about unit testing: * "I don't have time to write tests, I just want to code" * "I don't have time to run tests. If they're failing I'd have to fix it" It also mirrors the 19th century c…

The problem is that most tests break frequently not because of bugs, but because of some environment dependency that changed. The tests break and we have to fix them not because a bug was found, but because something else changed that affected the test. Most of the time spent with tests is not to catch bugs. When was the last time that you caught a significant bug with a test?

>When was the last time that you caught a significant bug with a test?

This happens at least once a week, and that's just for me on a ~15 person team.

I work on a compiler and most of our tests are in terms of user visible effects from running snippets. If any of them break, it's either a rare test harness failure or a real bug.

Whether or not any particular spec violation is significant is another matter of course, but they're definitely genuine bugs.

Re: Confessions of a programmer: I hate code review (2010)

#149

Post by let_var » Mon, Oct 29, 2018, 7:53 PM UTC

Code reviews have been both a useful medium to share new learnings among team members that actually makes the code elegant or perform better, and an endless threads about conventions and nitpicking.

I think it makes sense to set premise or template for a good peer reviewing exercise. New team members should be educated about these guidelines.

Re: Confessions of a programmer: I hate code review (2010)

#150

Post by McP » Mon, Oct 29, 2018, 7:55 PM UTC

Eleopteryx wrote: post #129

addicted wrote: post #103

Earlier quoted context omitted.

Further, eliminating bugs caused by any individual code commit is not really the goal behind code reviews in the first place. Finding bugs that the user notices on immediate use is an indictment of the QA process (whether manual or automatic). Code reviews help maintain sanity in the code base, and are useful for sharing knowledge (both in terms of quality of code, and ensuring that there are at least 2 people who ha…

>Code reviews help maintain sanity in the code base But why delay the shipment of features/fixes to the customer for any of these reasons? If code quality and human understanding is the actual rationale, it could make more sense to review code after it has shipped. In that way, it would cease to be a bottleneck for the developers or customers. Additionally, any bugs that appear in production could be accounted for du…

Because in most environments it's unusual that a developer would go back and change the code after shipping it -they'll have e.g. another high priority bug to fix. Reviewing and refactoring code prior to shipping helps to keep development going sustainably, reducing build up of technical debt.