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

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

#81

Post by stefan_ » Mon, Oct 29, 2018, 9:36 AM UTC

xiphias2 wrote: post #8

,,Reviewing code is one of those things that I would enjoy if I had infinite time, but that I find a nuisance when I don't.'' I can't really imagine any work that can be more important than code review for other people. This is the way Linus can have a 100x leverage on the Linux code base. This is the most important and hardest work in the Bitcoin code base as well, probably apart from cryptography research. I think…

The compensation, in Linux anyways, is that others will review your code if you review theirs. So the motivation to review stuff on the mailing list is purely that you need someone to review your code to get it in.

It sounds like quid pro quo but I found the level of review (e.g. for the DRM subsystem) to be excellent, even if it's one AMD eng reviewing some other AMD engineers code in the AMD specific code.

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

#82

Post by lostcoder » Mon, Oct 29, 2018, 9:47 AM UTC

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 by achievement of business goals and revenue.

For every anecdote you have about code reviews finding bugs, I have my own anecdote about 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.

So yes, code review IS optional if you can justify it.

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

#83

Post by mfukar » Mon, Oct 29, 2018, 9:53 AM UTC

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…

I like how you emphasise the "IS" instead of the "if".

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

#84

Post by watwut » Mon, Oct 29, 2018, 9:58 AM UTC

watwut wrote: post #80

I hate code review too, mostly power plays it brings. I had reviewer forcing me to produce code I considered bad - but the prolonged discussion before deadline would mean more wasted time so I did not argued. I have seen code review block code for subjective differences in variables naming (e.g. no convention was broken and both names were fine). I have had code reviewer demanding change, after I implemented exactly…

The core problem is that code reviewing is leadership, supervisory/gatekeeper function as any other, kind of like editor and should be treated as such. Even if reviewer and coder are formally at same footing or switch functions, it is momentary it. So, treat it as such and expect people to treat it as such. Meanwhile, people in tech talk about it in idealistic terms that are sometimes true, but often not.

1.) Don't use code review to teach juniors, talk with juniors in advance and supervise them as they go. If they learn about major architectural problem in code review, you are senioring wrong.

2.) Don't try to tech colleges in code review or use code review to force new standards on ad-hoc basis. Talk about issues in standards before, teach colleges as you go around your day without holding pull requests (and their weekends in work) hostage. If your only communication about these issues is code review, you are doing it wrong.

3.) Don't assume that code reviewer is automatically right just by its function and don't let code reviewer to steal responsibility from coder. Don't assume opposite either (through assuming opposite is better). Don't make discussion a taboo.

4.) Difference in opinion is normal and does not mean one of the people in conflict is idiot.

5.) Passive aggression does not belong to code review. Don't lie in code review comments either. "This is objectively horribly wrong" counts as lie if you are using it to push for personal preference and not ready to defend it. If you say or imply in your code review comment that someone is lesser coder, it is ok for that person to challenge the assumption and defend himself.

6.) Keep your own emotions out if as much as possible. If the code is making you angry, is it objectively bad or just something you are personally unused to (new syntax can be annoying even if it is good)? Distinguish the two.

Code review should be about one things only: Is this good enough to go to codebase? Is this required change a blocker or minor thing (variable naming)? All the other functions can be a bit helped by review, but absolutely should not be seen as review purpose.

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

#85

Post by kochthesecond » Mon, Oct 29, 2018, 10:00 AM UTC

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…

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 "I did not understand this, can you please explain?" Quite a bit if a part is confusing.

I try to invite to a discussion, not demand doing it to my taste and make it clear a lot of the comments are suggestions for future work.

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

#86

Post by jyounker » Mon, Oct 29, 2018, 10:13 AM UTC

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 view of pairing as a form of continuous code review is not a new insight, but it's also not a widely recognized or appreciated one.

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

#87

Post by danmaz74 » Mon, Oct 29, 2018, 10:14 AM UTC

pytester wrote: post #68

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…

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

Even dogmatic people can learn not to be dogmatic.

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

#88

Post by betageek » Mon, Oct 29, 2018, 10:15 AM UTC

mfukar wrote: post #83

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…

I like how you emphasise the "IS" instead of the "if".

For every anecdote you have about code reviews finding bugs, I have my own anecdote about 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.

Sounds like you have bigger problems than code review!

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

#89

Post by zimpenfish » Mon, Oct 29, 2018, 10:22 AM UTC

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.

> Code review doesn’t catch bugs.

It can but it's definitely not a 100% surety (as some people seem to think.)

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

#90

Post by arnvald » Mon, Oct 29, 2018, 10:23 AM UTC

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…

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.