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

61–70 of 165 posts

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

#61

Post by stefs »

i agree with the author in that i personally don't like code reviews much as well, though i agree they are valuable.

to be more specific though, i don't like "remote" or "detached" code reviews much; "over the shoulder" code reviews are a completely different thing. as you can actively communicate with the author or the reviewer, it tends to reduce the bike shedding a lot and the value of positive feedback increases.

i guess, ultimately, it depends on the team; if you're operating well together, classic code reviews might work as well. if the team is new or the members have varying styles or levels of exerience, over-the-shoulder reviews tend to pay off significantly.

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

#62

Post by golergka »

How about a novel concept: there will always be some parts of your job that you don't enjoy. Tedious things, boring things, annoying things. All the different check lists, beurocracy, reports, task tracking - there are a lot of annoying little stuff that we sometimes have to do.

Some of those things can be optimized or automated away. But some of those things can't: turns out, although they're completely irrelevant 98% of the time, they're completely critical in the remaning 2%, and nobody knows what those 2% are, so we're stuck with these practices.

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

#63

Post by messit »

I've worked in teams bad enough to make code reviews a real pain.

I've seen pretty horrible code constructs that actually seemed to work without bugs. What should I say? That the author of that piece of code should completely rewrite it to my taste? It would not only frustrate him/her but also slow down the entire team. So I often accept what is rubbish IMAO. And not me alone, have you ever seen two code-buddies in a team always doing each others code review and(quickly) accepting and merging everything they produce before anyone can make a comment?

I've had numerous pointless discussions about why I wrote a specific piece of code where (a junior dev) the assessor just wanted me to do things by the book as he learned it, not really understanding why I did it that way. But there was no way to convince him, it ended up in an unresolved conflict that I only could solve by messing up my code to his taste.

All these pointless discussions that particularly affect the relationship you have with your co-developers in the team you work in, are not always fruitful and can be quite destructive if you're not careful enough.

I do believe code reviews can be very important and valuable, but at the same time I've seen huge amounts of wasted time and frustration for nothing. It's not always and only a good thing is my conclusion, it depends on the variables.

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

#64

Post by seer »

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…

Not that this applies to your particular situation of course but I have seen places where that duplication was actually warranted - especially in css. E.g. some code was extracted and reused when it was not meant to, just drying the code up, but those things turned out not to be the same thing and it was rather painful when we had to change them independently.

As to your particular case - I’m sure the politics of the situation were pretty bad but I’ve solved situations like this by having two separate commits/prs one that uses the old style to fix a problem so the change is obvios, the other to change all the old style to the new style of code. Helps the reviewer understand what’s happening and move the PRs along. Though this was in a much more liberal code review environment of course ...

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

#65

Post by bryanrasmussen »

seer wrote: post #64
bryanrasmussen wrote: post #39

Earlier quoted context omitted.

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…

Not that this applies to your particular situation of course but I have seen places where that duplication was actually warranted - especially in css. E.g. some code was extracted and reused when it was not meant to, just drying the code up, but those things turned out not to be the same thing and it was rather painful when we had to change them independently. As to your particular case - I’m sure the politics of the…

no, it would not have worked in this situation. the problem was really the reviewer who could not be gotten around.

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

#66

Post by therein »

messit wrote: post #63

I've worked in teams bad enough to make code reviews a real pain. I've seen pretty horrible code constructs that actually seemed to work without bugs. What should I say? That the author of that piece of code should completely rewrite it to my taste? It would not only frustrate him/her but also slow down the entire team. So I often accept what is rubbish IMAO. And not me alone, have you ever seen two code-buddies in a…

> I've had numerous pointless discussions about why I wrote a specific piece of code where (a junior dev) the assessor just wanted me to do things by the book as he learned it, not really understanding why I did it that way. But there was no way to convince him, it ended up in an unresolved conflict that I only could solve by messing up my code to his taste.

I've left the place I worked at for over 3.5 years over stuff like this adding up. Starting at my new job in a few weeks, getting paid more than I did at my previous job and will be working with people more senior than in my previous team. All in exchange of the rapport I've built over 3.5 years.

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

#67

Post by rasteau »

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…

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

One concern here is you may ask questions whose answers are then given to you, and you alone. If it's likely someone else in the future will have the same questions, the "answers" should be in the form of improved code.

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

I'm not clear on the distinction between "difficult sections" and "unclear areas"? Either way, one of the outcomes of the review should be to make life easier for the folks that follow us.

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

#68

Post by pytester »

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

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…

>The worst code review comments are “this isn’t performant/idiomatic/maintainable, fix it” or other statements of opinion presented as facts.

This behavior is usually symptomatic of an excessively dogmatic outlook. That is, I don't think it's the review style per se that's the problem - it's the person.

I find developers like this nearly impossible to work with. They won't just unknowingly write poor code themselves (dogmatism typically leads to bad decisions which leads to really poor code) they will likely try and force you to do the same.

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

#69

Post by bryanrasmussen »

rasteau wrote: post #60
bryanrasmussen wrote: post #39

Earlier quoted context omitted.

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…

Yikes! Demanding a bug fix also remediate unrelated, extant style violations is a good way to ensure nothing improves.

right. I said that to him and to the frontend lead for the whole organization (very big place) - codebases this big you can't have that requirement, do it with a small codebase/team who all sit in the same room and know what's going on sure. 10-11 teams spread around with lots of different programmers, many also remote, it can't be done. But no dice.

In fact I had people specifically recommend me at times, don't do that because when it hits review with reviewer X it will be a problem. Sometimes you had to butter them up asking their opinion before (and that wouldn't always work because sometimes they would change their mind) anyway I better shut up and get to work, I can feel the bitterness returning!

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

#70

Post by cimmanom »

Vinnl wrote: post #26
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 doi…

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?
Post reply on HN