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

31–40 of 165 posts

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

#31

Post by Vinnl »

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…

> where the author says they feel like code review is optional. If you think it's optional

I think these are two different things. The author says they don't enjoy code review, and that that probably is because it feels optional. From the post, I gathered that that's not what the author thinks - the first few paragraphs emphasises that they think it is an important part of the work. But apparently, that knowledge isn't enough to change the experience of reviewing.

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

#32

Post by falsedan »

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 submitted has the same result

2. share their personal preference for how they would have written it

3. explain why they prefer it over the code I wrote

4. not block the review on me rewriting the code to match their preference

I think it’s fine (and valuable) to comment “I hate this code, shipit” or (better) “ship it, if you change foo to bar then ”

The worst code review comments are “this isn’t performant/idiomatic/maintainable, fix it” or other statements of opinion presented as facts. This kind of faux-objective feedback is terrible, since a fact can be argued against (or could be wrong). A statement of opinion like “I don’t like it” or (best) “I had to think really hard before I understood it” is great, since that’s what someone diving into the code for a maintenance ticket/incident will be thinking but won’t have the original author on hand to walk them through the code.

Linking to articles or references that explain why you prefer a particular implementation also gives the reviewer a learning opportunity & lets them save face when they post a follow up CR “thanks, didn’t know that”.

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

#33

Post by edmundhuber »

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…

I agree that most code review change requests are style-related.

One reason I can think of this happening is that in order for one's brain to focus on just the parts of the CR that matter, stylistic oddities need to be sorted out. Ideally after some time, a reviewee would stop opening PRs that contradict the basic stylistic standards that the group adheres to (and thus stop wasting everyone's time). Ideally there would also be a linter to get everyone to that point as quickly as possible.

Imagine if you were running an assembly line, and every person in every shift did their welds significantly differently. It would be a waste of time for the inspection workers to have to continuously remind everyone that it's hard to check quality on a variety of different welding methods, rather than just one or two. It would be a better use of everyone's time to just agree on what kinds of welds are acceptable.

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

#35

Post by closeparen »

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

Mediocre code review is a human linter; decent code review catches obvious bugs; good code review catches subtle bugs; great code review teaches you something. I’m always excited to learn (or teach) when there’s a better way to write something. Even if not adopted, the surrounding discussion is a boon to future readers.

If people are giving crazy suggestions, or resenting good ones, something is seriously wrong with your team.

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

#36

Post by drej »

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…

Fascinating numbers. Do you know if that 95% number could be brought down by tooling? We have deployed fairly strict code style guides + included some linters and other tools to watch over code quality to reduce these conversations about style, but it obviously doesn't catch everything.

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

#37

Post by _shadi »

In one of the teams I worked in we had a policy to mitigate some of the things we found inefficient in our code reviews:

1- anything done in a pair doesn't need a code review.

2- any story that is more than 3 story points should be worked on in pair.

3- if you are doing a code review and you find something that needs to change you don't leave a comment, you update the branch and ask the original branch owner to review your new commit.

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

#38

Post by ankit219 »

I think the author is talking about a 'Forced Code review' because it is supposedly a good practice to have a code review before you put something into production (which it is).

The current practice is a coder submitting the entire code for review, and getting comments on every aspect that the reviewer feels like. This is not an efficient process as it takes up a lot of time, but reviewer also put in a position as to 'How would he solve the problem the current code has already solved'

I feel a good code review process should have two components: 1/ Self-review. A team/company should agree on baselines/conventions that any code shipped by them should have, and ensure that any code not corresponding to that will be rejected. This review should be done by developer himself, rather than a reviewer.

2/ A code review by an experienced dev who needs to check for libraries/custom functions that are used might not break something somewhere else, or wont work because some other change is going somewhere else, that they know of. This way, potentially buggy code can be avoided, and process could be faster as well. Ofcourse, if they want to provide feedback on a better algo, they can, but should be optional

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

#39

Post by bryanrasmussen »

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 over the place and this can have side effects. The diff shows it as my code but if you look you can see it is actually the old code just moved to right place.

Those are the rules fix them.

since this code was in a big release of stuff and none of our stuff was allowed through unless I changed one em to rem even though I could not be sure there wouldn't be problems I went ahead and rolled back my changes.

fixed overflow bug other way. no code reuse ever in that part of codebase.

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

#40

Post by weeksie »

Pull request reviews are for merges into production, (or staging depending on your QA process) but you should always have a develop branch you can thrash on in the case that you are pushing a feature that is being worked on by another team (e.g. front/back end)

Fix the workflow. Reviews are the best quality control measure out there, perhaps even better than unit tests.

Post reply on HN