Live data from Hacker News

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

blog.nelhage.com

151–160 of 165 posts

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

#151
In other jobs, I accept that I'm occasionally going to be waiting. The goal is to produce value at the organization level, and that just isn't compatible with each individual operating at 100% efficiency at all times. It's egotistical of programmers to think that they should. It's unfair of managers to expect this of them. Amdahl's Law applies to humans, too.

The issue is procedural. If you hate that code review of an API implementation might require changing the interface, then you should do a review of the API design before you implement it. That should be the case with every phase. Any time you have to perform multiple steps and worry that review of a later step might cause an earlier step to be invalidated, that simply means you're reviewing too late, at too coarse a level.

Or not. Adding more reviews keeps the big-O factor down, but increases the constant factor. As an organization, you can pick if you want individual contributors to never have to backtrack, or if you want them to be able to use speculative execution. One way is more predictable, and the other way can be faster if you're good at guessing right.

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

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

...and if any of those four are not in place, it can be a nightmare. At a rough guess, less than a 1-in-16 (2^4) chance of a non-aggravating code-review process at any given company.

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

#153

Earlier quoted context omitted.

These are all good tips, but I want to stress the importance of (6). Much of what the original post is talking about can be improved by adopting the habit of making small, incremental changes. As the author, you no longer have to wait "in the order of weeks" for the feedback and a potential rebase should pose little risk. As the reviewer, you don't have as much mental burden and most reviews can be done "in-between"…

Conceptually, I completely agree with (6). Practically, though, I found it really hard to do in some situations. Specifically, when doing something where the solution is well known for the beginning, creating PRs which are small and easy to understand is easy. When instead I have to solve difficult problems for which I don't already know the solution beforehand, and which require writing quite a bit of complex code -…

The answer to your problem of "large commit" is this:

1) Create "research prototype" of your solution. But do not commit it or commit it into separate branch (not master).

2) Create a plan how to split that code into smaller steps. Separate refactorings that will support your new feature - from your actual feature (that changes functionality).

3) Commit refactorings separately from your main feature commit. Keep every refactoring commit as small as possible).

4) At the end commit your functionality changing feature. If possible, split it into multiple commits too.

That should help with code review.

Remember, your team will have to code review your code multiple times:

1) When you are making your commits.

2) When your code reviewer reviews it.

3) In the future, when you or other team members maintain your code and try to understand why you created code that way.

So - optimize your commit to reduce code review time (even if it increases initial code writing time).

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

#154
post #108

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?

> When was the last time that you caught a significant bug with a test?

Constantly. This is an extremely surprising question to me, our unit tests catch bugs all the time.

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

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

Did you consider fixing "em -> rem" (in a separate commit) before you start applying your own fix to that code?

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

#156

Earlier quoted context omitted.

Conceptually, I completely agree with (6). Practically, though, I found it really hard to do in some situations. Specifically, when doing something where the solution is well known for the beginning, creating PRs which are small and easy to understand is easy. When instead I have to solve difficult problems for which I don't already know the solution beforehand, and which require writing quite a bit of complex code -…

The answer to your problem of "large commit" is this: 1) Create "research prototype" of your solution. But do not commit it or commit it into separate branch (not master). 2) Create a plan how to split that code into smaller steps. Separate refactorings that will support your new feature - from your actual feature (that changes functionality). 3) Commit refactorings separately from your main feature commit. Keep ever…

Thank you for your answer, but I don't think that would have been feasible - our employer called me exactly because the team was very slow, and adding so many additional steps would make me very slow too.

I'm now changing contract, hopefully with the new team it will be possible to find a better compromise. For example, I offered to walk the reviewer through my code, which I think would have helped a lot, but this was refused...

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

#157

Earlier quoted context omitted.

The answer to your problem of "large commit" is this: 1) Create "research prototype" of your solution. But do not commit it or commit it into separate branch (not master). 2) Create a plan how to split that code into smaller steps. Separate refactorings that will support your new feature - from your actual feature (that changes functionality). 3) Commit refactorings separately from your main feature commit. Keep ever…

Thank you for your answer, but I don't think that would have been feasible - our employer called me exactly because the team was very slow, and adding so many additional steps would make me very slow too. I'm now changing contract, hopefully with the new team it will be possible to find a better compromise. For example, I offered to walk the reviewer through my code, which I think would have helped a lot, but this wa…

> additional steps would make me very slow too

Your real choices are: "fast and buggy" vs "slow and correct".

What is your preference?

> I offered to walk the reviewer through my code

Walking reviewer through your code is a good exercise. Code review of complex change should be done in interactive session (code reviewer + coder). There is so much to learn for both sides in such session.

https://dennisgorelik.dreamwidth.org/161605.html

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

#158
post #71

> If I spend a day doing nothing but reading code reviews, I'll end up feeling unsatisfied and unproductive. Because code review feels fundamentally optional -- even though I believe it's beneficial, it's something we've chosen to do, not something that we absolutely have to do in order for the project or business to keep operating -- it's more frustrating to find myself spending a large amount of time on. This is wh…

I'll add that code reviews are vulnerable to garbage in, garbage out. Code reviews the are magically 100% efficient at catching will still be expensive in terms of time if all incoming code is garbage. By reducing upstream garbage, code reviews become less expensive, making more time available for anything else, including coding.

Fortunately, code reviews improve quality of incoming code: both original coder and code reviewer learn how to code better during code review.

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

#159
In my team, we have started using these rules to address the code review lag-time:

* While you are waiting for code review, review someone else's code

* Aim for reviewing two CR:s per CR you submit.

This has helped us both keep on top of the CR queue and to have something that is not too distracting to do while waiting for review.

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

#160

Earlier quoted context omitted.

Thank you for your answer, but I don't think that would have been feasible - our employer called me exactly because the team was very slow, and adding so many additional steps would make me very slow too. I'm now changing contract, hopefully with the new team it will be possible to find a better compromise. For example, I offered to walk the reviewer through my code, which I think would have helped a lot, but this wa…

> additional steps would make me very slow too Your real choices are: "fast and buggy" vs "slow and correct". What is your preference? > I offered to walk the reviewer through my code Walking reviewer through your code is a good exercise. Code review of complex change should be done in interactive session (code reviewer + coder). There is so much to learn for both sides in such session. https://dennisgorelik.dreamwid…

> Your real choices are: "fast and buggy" vs "slow and correct".

It's not a binary choice, it's a dial. And anyway, after more than 30 years of programming experience, I can tell you that I can write mostly correct code by applying diligently the pyramid of tests. In my experience, code reviews catch very few bugs that weren't revealed by testing, if any.

Much more than correctness, I find them useful to verify and improve the readability and maintainability of code - which are important goals, but as always the cost needs to be measured against time and cost constraints. Depending on the project, being twice as slow to accomodate the reviewer can or can't make sense. What I'm looking for is more of a 80/20 solution: can I be 20% slower, and still get 80% of the benefits of code reviews?

Post reply on HN