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

101–110 of 165 posts

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

#101

Post by Vinnl »

bradleyjg wrote: post #99
Vinnl wrote: post #26

Earlier quoted context omitted.

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

There are definite upsides to having a detailed written record of a code review, but in terms of enjoyment if I have some code to review I read though it, take some notes, grab a meeting room and have a discussion with the person that wrote the code. I find it far less tedious than having to write up my critiques in detail. Then the written review in the tool can just be terse reminders to the things we discussed. I'…

Hmm, that does sound like a good idea that would make it a lot more enjoyable, with the added bonus of being able to use tone of voice and facial expressions to prevent remarks from being taken the wrong way.

I'm not currently in a position where I share an office with people whose code I review, but when I am again, I definitely want to try this. Thanks for sharing!

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

#102

Post by wool_gather »

lostcoder wrote: post #82
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…

I'm fortunate enough at my company to not have to submit to the code review bureaucracy - and I can justify it by having metrics which indicate that my code ships with fewer bugs than the code of other teams which do go through code reviews. I think everyone should have some level of freedom to work in a manner which works best for them, rather than forcing everyone into the same process. Success should be measured b…

> people writing code going through a thorough code review process and the code crashes the first time an end user tries to use the functionality.

This is not an argument against code review. This means that the change wasn't even ready for code review. Whoever put up those patches skipped the step before it, where they verified the functionality of their own code.

Code review shouldn't -- can't -- be a time for someone else to mentally (or actually) execute the code and sign-off on it being bug-free. It's a sanity check for read-/maintain-ability. You can generally validate that the patch isn't doing something stupid or dangerous, but it's 100% on the submitter to ensure that their code works. If you have people putting code up for review without testing it first, then you have a bigger (people) problem, that code review will never be able to help with.

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

#103

Post by addicted »

arnvald wrote: post #90
lostcoder wrote: post #82

Earlier quoted context omitted.

I'm fortunate enough at my company to not have to submit to the code review bureaucracy - and I can justify it by having metrics which indicate that my code ships with fewer bugs than the code of other teams which do go through code reviews. I think everyone should have some level of freedom to work in a manner which works best for them, rather than forcing everyone into the same process. Success should be measured b…

What's the justification here though? Saying "my code is better than other teams' even without review" does not prove that reviews are not helpful - maybe if you get your code reviewed you'll ship even fewer bugs, and if other teams don't review they'll ship even more bugs.

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 have some idea behind the code written). It provides an opportunity to have it read by someone who doesn’t share the same priors as you and helps you identify sections of code that may be confusing for another developer (including future you).

If Code review does lead to the identification of bugs that’s just a bonus.

There’s a reason Code review does not have a “run the code and test it” component to it and is entirely focused on code, and not whether it runs correctly.

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

#104

Post by v_lisivka »

bryanrasmussen wrote: post #39
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…

so another story here: code review mandatory at last place I worked, the place had a policy of any code you push in that does not match the guidelines must be changed. Guidelines was that no em allowed, must use rem. Old part of codebase assigned to me, I found some things in CSS I improved (reuse of code, a small overflow bug) Got comment - you need to change em to be rem. I can't do that because there is em all ove…

Oh, no! You copy-pasted code, with zero knowledge about it structure, and got caught. It's sad.

Next time use throw-away account.

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

#105

Post by wool_gather »

torben-friis wrote: post #79
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…

> 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). There's a potential problem with that idea: The assumption that, if several programmers invol…

You're right. I'd say it's an aspect of professionalism to get better at this as a reviewer. One of the things you need to try to do when you read somebody else's pull request is to put aside your ego and focus on the code.

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

#106

Post by vfc1 »

Systematic code reviews is really a complete subversion of the agile spirit. Agile was about doing what makes the most sense for the project at any given time, and code reviews don't make sense all of the time, far from it.

I worked on projects with systematic code reviews, and while the reviews hardly caught any issue, they did add significant delays and endless discussions with some very unreasonable people that would systematically question every single line of code for no good reason.

I had to submit reviews and wait a day for them to be approved for things such as 3 lines of CSS changed.

Code reviews should be left for occasions when the code actually needs some review: for example, the developer changed a part of the code that he/she is not familiar with and wants the code to be checked by someone who is experienced in that part of the system.

Code reviews yes, but only when it really makes sense, and not by default.

Some of the things I've seen regarding code reviews are just way too absurd and it just adds to the overall demotivation of the team to have to do something absurd every day just because its the rules.

Most of the time, these procedures are put in place by non-technical managers who have usually not coded themselves a single line of code in their lives.

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

#107

Post by v_lisivka »

caymanjim wrote: post #96

I agree with everything in the article. My biggest problems with code review are: 1. The reviewer usually lacks the context to truly review the solution and all its implications, and instead gives only a cursory review focused on style and trivialities. Sometimes they're familiar enough with the code to give an insightful review, and there's benefit to commenting on even the small things, but I've seen countless majo…

1. You need to put context into comments. Code is written once but read multiple times, so you should optimize for later case.

2. "Too late" is when bug hits the client and affects income. If not, then it's not "too late", it's just "a bit late". 2-3% in effort increase to decrease number of bugs (increase customer satisfaction) by 15-20% is huge win. If you will not do that, your competitors will do that instead of you.

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

#108

Post by vfc1 »

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?

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

#109

Post by sethammons »

vfc1 wrote: post #106

Systematic code reviews is really a complete subversion of the agile spirit. Agile was about doing what makes the most sense for the project at any given time, and code reviews don't make sense all of the time, far from it. I worked on projects with systematic code reviews, and while the reviews hardly caught any issue, they did add significant delays and endless discussions with some very unreasonable people that wo…

While I agree with the spirit, I disagree in whole. "One line config change? I don't need a code review." At our org, even that needs a CR. It takes a few minutes generally to request it in our chat room. Usually, the other person sees no issue and you get their stamp that guards the build process. However! Multiple times (a small percentage), the person asks for a clarification on it and it turns out that a conflicting config needs updating or a comment is warranted or they remind you that this change will (maybe minorly) affect another team and asks if you reached out to let them know.

Can it get in the way? Yes, it can. So as a team, learn how to streamline. For us, usually someone is available for a "quick cr." Larger ones take longer. Maybe some changes are too small, but for our team (no CSS), we've not found a consistent candidate.

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

#110

Post by cimmanom »

Vinnl wrote: post #77
cimmanom wrote: post #70

Earlier quoted context omitted.

While you wait for code review you work on another ticket. What kind of screwy engineering process assumes that code review is instantaneous and doesn’t give you something else to do in parallel?

Nobody expects code review to be instantaneous - that's the problem. Working on another ticket involves context switching, and context switching negatively affects the productivity. Sure, that downside is probably offset by the benefits of code reviews, but that was exactly what I said: receiving a review is beneficial, it's just a bummer that there's the downside that it's not instantaneous.

You're going to be context switching at the point you make the pull request anyway.

And if you structure your day well, you can do the rest of the context switching at a point when it's not a burden anyway - such as making changes in response to code review first thing in the morning before returning to your other tickets.

Post reply on HN