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

21–30 of 165 posts

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

#21

Post by DubiousPusher »

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 surprised how much time this saves.

4) Don't struggle with difficult sections of other people's code unless you have a vested interest in remembering this specific section of code. Ask about unclear areas.

5) Trust your CI process. Don't try to consider every edge case for your reviewee.

6) Develop a culture of small work units. Encourage people to integrate every day or two. This means reviews are small and easier to grok.

Code reviews are not a guarantee of correctness. They are another tool. We should find a good trade off between effectiveness and cost to get the most value out of them not strive for perfection.

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

#22

Post by mycelium »

falsedan wrote: post #18
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…

Code review doesn’t catch bugs. Sure, sometimes an experienced dev can spot a bounds error or race condition, but CRS are for knowledge sharing and style adherence.

Completely disagree. I catch bugs, and have my bugs caught, at least a few times a month.

Beyond the "this will likely deadlock without a timeout", "this does not clean up after itself on exception", "if the cache is not hot, this operation you assume is 1 sec will take 1 min" type thing — one of the strongest points of code review is having a conversation about "this code is hard to understand" and turning code that's correct if you think about it really hard into obviously correct code.

It takes a reasonably strong reviewer to get there though, and that's not always possible.

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

#23

Post by tristanperry »

geoffmunn wrote: post #3

Code reviews, aka opportunities for dickheads to demonstrate how clever they are. "Did you know you can replace that entire function with a regex?"

I think this comes down to team culture and morale.

This certainly does happen in a team with a bad culture/morale, but IMO it's just a symptom of the wider issues.

But yep, code reviews do make situations with bad morale worse IMO.

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

#24

Post by terapixel »

Of the choices that he proposes, #2 seems like the obvious solution to me - but he writes it off as too difficult.

Phabricator’s stacked diffs [0] works wonderfully well to support this exact workflow (it isn’t clear from the post which versioning system the author uses and what workflows it supports - maybe I missed it?)

[0] https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/

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

#25

Post by barbs »

zaksoup wrote: post #6

I've found that favoring full time pair programming and not reviewing work that was soloed upon unless the author explicitly asked for it because of self-identified uncertainty or caution has resulted in a better workflow with little to no actual hit to quality. Additionally, proper test-driving and testing practices are incredibly effective ways to enforce high quality code. Again, this comes down to culture and pra…

Pair-programming 100% of the time sounds like a nightmare to me. I need uninterrupted focus - something I can't do with someone else there constantly, even if we're working on the same thing

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

#26

Post by Vinnl »

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…

I think in such a situation, receiving a review is great, even though the downsides mentioned in the article ("what am I going to do while I wait for the review to come in?") are still presents.

However, it doesn't do away with how unsatisfying performing a review is. If I spend a day doing code reviews, that is not an enjoyable day to me. This is regardless of how useful I feel it is; it's just not a job I enjoy doing. It'd be nice if there was some change we could make to make it a more pleasant aspect of the job.

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

#27

Post by Sharlin »

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.

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

#29

Post by petetnt »

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…

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.

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

#30

Post by lowercased »

mnd999 wrote: post #19
lowercased wrote: post #11

Rant... Was nominal lead on a smallish project a couple months back, with 2 other folks. Worked with one before, one I hadn't worked with before. We're all remote, and the 'new' guy started doing 'code review' on code before he had a running environment. We'd taken over another codebase, and he was pretty up-front about wanting to adopt/enforce certain stylistic standards, which... to me, I don't particularly care ab…

Doesn’t sound to me like you are the problem. Sounds like your colleague has lost sight of the end goal and needs to refocus on what you’re supposed to be delivering. (Unless the whole project was to reformat the code?)

I'd later learned through someone else who'd worked with 'new guy' that they'd had similar problems, and apparently he only really works well on his own. I'd been 'given' him on this project because the first person wasn't really sure if he (first person) was at fault, or if 'new guy' was. I was the test case that confirmed.

By no means was the purpose to just 'reformat' (far from it!) :)

I have wrestled with this a couple times before over the last ... 15+ years. When working with other folks, I'm normally not in 'full time w2' arrangements - it's usually shorter term contracting, so the dynamics are a bit different. That may also be why the style/formatting is usually less important to me. As with the project above, I'd seen many well-formatted projects with descriptive variable names that don't function. And... seen horrific spaghetti code that "just works" but people are afraid to touch it.

Finding that middle ground is a never-ending journey, but I feel I've got a decent sense of how to get pragmatic results, and it usually involves repeatable sample data, repeatable processes and tests and/or docs. Building those as a foundation will make stylistic changes a breeze, because the confidence to make "trivial" changes will be there. Coming in to new codebases with no tests, sample data or processes should be considered scary/risky, and when people don't seem to have an understanding of the risks involved, I get even more scared.

It took us 9 weeks on this project to get to a point where we made a first push out to production (and even then we ran in to a few small bumps). But we'd practiced with tests (only have dozens, not hundreds at this point) and pushing to staging, and as the client reviewed... we kept finding more and more bugs - things that had existed in the code for years that we were seemingly the first people to exercise in front of the client. I'm sure their confidence in us was bruised a bit - we apparently just kept showing bugs(!) - but it feels like we provided more of a base in 9 weeks than we inherited from the previous 4 years of teams (automated builds, sample data, automated unit tests, automated browser tests, etc).

Just did this a couple days ago and it's still very fresh/raw in my mind :)

Post reply on HN