Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

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

blog.nelhage.com

Post reply on HN
Print view View original

41–50 of 165 posts

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

#41

Post by goto11 »

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

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

#42

Post by mycelium »

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

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

#44

Post by ereyes01 »

"Good cooking takes time. If you are made to wait, it is to serve you better, and to please you."

^^ From the Menu of Restaurant Antoine, New Orleans, quoted in the beginning of The Mythical Man Month.

Good engineering takes time and deliberate effort. Invariably, it will sometimes be tedious, but it is for the benefit of the product. IMO that includes code reviews, and/or pair programming.

In my experience, the biggest source of friction is in comments over style and reviewers/reviewees that have an obstinate and opinionated stance on the matter. In my current project, we resolved this by simply requiring everything passes a linter (gofmt and ESLint in our case). Follow the linter, learn to like it, end of discussion. There's still structural / design style to fight over in reviews, but those are healthy fights IMO.

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

#45

Post by neathack »

DubiousPusher wrote: post #21

Some simple tips I use to make this better. 1) Anything cosmetic gets a "nit" added to the comment. 2) When a review looks like mostly assets or boiler plate, I look through quickly for anything glaring and approve while noting my assumptions that the author has run the build and verified the additions. 3) If I'm in a hurry and the author is available I grab them and do an over the shoulder review. You might be surpr…

These are all good tips, but I want to stress the importance of (6). Much of what the original post is talking about can be improved by adopting the habit of making small, incremental changes. As the author, you no longer have to wait "in the order of weeks" for the feedback and a potential rebase should pose little risk. As the reviewer, you don't have as much mental burden and most reviews can be done "in-between" bigger tasks, e.g. in the 15 minutes before a meeting or before lunch, etc.

As for 1) I highly recommend to automate that away as much as possible. A strict syntax guideline and a toolchain that enforces it will go a long way.

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

#46

Post by walterstucco »

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…

1 - hospitals in US still have to get their employees to wash hands in 2018

2 - washing hands had a major impact on saving lives, tests and CR have not improved coding in a measurable way

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

#47

Post by Double_a_92 »

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

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

#48

Post by austincheney »

When the right processes are in place I am huge believer that most of the concerns raised from code reviews can be fully automated. I usually find people suck up code review time with code style or concerns of conformance to team standards. This is really basic stuff that sucks the life out of people and isn't productive.

For me the residual benefit of code reviews is to invite people to examine how the code works so that it can be improved or simplified. By improve I mean objective concerns like execution performance, reduction of instruction size, security concerns, and so forth. I expect a very critical analysis. Don't be afraid to hurt my feelings because this code is about to go to production.

Unfortunately, often times the proper automation controls are either not in place or the developers are fragile timid souls, particularly in group settings. Code reviews can be wonderfully constructive, but many times aren't.

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

#49

Post by jatins »

My only advice to reviewers is: If you say "do X", also explain the "Why?" behind it.

Other than my only advice, these points are also helpful: - Zero Code Style, Formatting Comments should be allowed on reviews. If it passes the linter, it's good. - Re-naming classes/variables comments should be kept to minimum. Unless someone is using obviously bad names like `a`, it's probably a fine name.

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

#50

Post by Sharlin »

erwan wrote: post #7

I love code-reviews. With the right people, they can be useful both to achieve high-standards of code quality and also improve your own engineering process. The requirements are quite high, I think you need some combination of the following: 1. Both reviewer and reviewee are focusing on getting the best outcome possible, in good faith and with generosity. 2. The reviewer concedes that there can be equally valid appro…

Definitely. Those points seem fairly obvious to me, but maybe I've just worked with very professional people.
Post reply on HN