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

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

#111

Post by sethammons » Mon, Oct 29, 2018, 12:38 PM UTC

vfc1 wrote: post #108

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?

For us, a failing unit test is almost always a bug. Lots of people write terrible unit tests. Too much mocking, asserting methods called, etc. Brittle. Test the edge of your public interface. Supporting methods only need specific unit tests if they are sufficiently complex. If your unit's interface changes or it's behavior changes, it should fail. That is a good thing.

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

#112

Post by tekkk » Mon, Oct 29, 2018, 12:42 PM UTC

LandR wrote: post #95

tekkk wrote: post #51

Only time i've come to hate code review was when my counterpart (reviewer or the author) has been overly pedantic with no hint of any empathy in hir writing. I do not know was it because of inherent insecurity or no previous experience writing in more friendly manner but boy oh boy i was boiling inside arguing about useless things. The socially awkward seem to be in high numbers amongst programmers which makes these…

I don't get this. A code review is a purely technical process. If he says your code is bad / wrong etc then that is a purely technical discussion that has to be had if you feel they are wrong. Why are you looking for empathy in code reviews? Why are you getting feelings involved at all?

This might had been a cultural thing between me and the other party. I don't mind being very to the point and strict about programming but when it comes off as condescending and "I know better than you" it's just something I can't stand. Or maybe a better example, they make pesky remarks on your programming style while ignoring all your writing about theirs. I think because the other person here was a bit older than me, not a senior dev but still old enough apparently, that he felt he had to prove something to the younger guy.

But yeah I'm going off the rails here. My point is that relationships are hard and even in seemingly neutral setting, a code review, tensions can arise. I do not know what to say on those occasions but getting snarkier and snarkier in comments is definitely not the way to go. Bad code reviews have been one of the most infuriating things I've come across my developer career.

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

#113

Post by sethammons » Mon, Oct 29, 2018, 12:45 PM UTC

fzeroracer wrote: post #10

For me, I've never had an issue context switching from various branches and PRs. After a moment or two of looking through my notes and my code I can quickly get back up to speed with what I was doing. The issue I end up taking with code review is cases where reviewers end up forcing their own stylistic choices to the detriment of my time. Insisting that all equal signs be lined up, insisting that variables must be sp…

Auto code formatters for the win. If that is not an option, maybe a line in the style doc that says "shall" vs "may" (ie, things that are and are not kick backable).

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

#114

Post by jakecraige » Mon, Oct 29, 2018, 12:49 PM UTC

terapixel wrote: post #24

Of the choices that he proposes, #2 seems like the obvious solution to me - but he writes it off as too difficult. Phabricator’s stacked diffs [0] works wonderfully well to support this exact workflow (it isn’t clear from the post which versioning system the author uses and what workflows it supports - maybe I missed it?) [0] https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/

Thanks for the link! I went down a small rabbit hole from this which ended up with a script that allows for similar style workflows even within the GitHub PR world:

https://twitter.com/jakecraige/status/1056835104440545280 https://gist.github.com/jakecraige/51f9583f1f55cc0ecccfaa11e...

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

#115

Post by btasovac » Mon, Oct 29, 2018, 1:08 PM UTC

dotdi wrote: post #78

I used to hate code-reviews but now I'm a convert after: (1) we brought in better people. It's fun to sit with competent people and trash-talk some code, and (2) after I pushed the team to use better tooling, which in our case is GitLab + Merge Requests. You can imagine the dark ages before that.

We're happy to hear that GitLab helped you improve your code-review experience! We documented some of the advice and best practices that might be useful when performing code review or having your code reviewed [0].

[0] https://docs.gitlab.com/ee/development/code_review.html#best...

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

#116

Post by vfc1 » Mon, Oct 29, 2018, 1:13 PM UTC

sethammons wrote: post #111

vfc1 wrote: post #108

Earlier quoted context omitted.

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?

For us, a failing unit test is almost always a bug. Lots of people write terrible unit tests. Too much mocking, asserting methods called, etc. Brittle. Test the edge of your public interface. Supporting methods only need specific unit tests if they are sufficiently complex. If your unit's interface changes or it's behavior changes, it should fail. That is a good thing.

think there is a lot of religious-like thinking when it comes down to tests: many people swear by them even against all the day to day proof to the contrary that they are really not that useful at the application level.

I think if we are shipping a framework to thousands of users its great that it has unit tests, but if its an in-house application built by a team of 10 developers, I doubt that the tests are doing anything useful.

But what happens is that these best practices that are used by developers of libraries are then used blindly at the level of application code, without pausing to think if those are indeed best practices in that particular context.

Developing software at the leaves of the dependency tree (the application) is very different than developing libraries. Some things are the same while many others are not.

The end result in practice is a lot of wasted time and effort spent at the application level writing bad tests that yield no results, all in the name of the testing religion.

Because that is what it really is, its an unprovable belief system with a set of rituals that you absolutely must follow blindly, or else.

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

#117

Post by oldboyFX » Mon, Oct 29, 2018, 1:15 PM 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…

> The better way to think about it is that code review simply isn't optional.

Of course it's optional. Tests are optional too. There are many projects and situations where trading code quality for development speed makes sense. Especially when you're building MVP's, experimenting, or trying to validate market demand.

It's also often not worth it on small to medium sized projects where only one person is working on the codebase (think 3-person project: designer, front-end dev, back-end dev).

Larger projects with multiple people working on the same codebase are a completely different story. In those cases code reviews are valuable and highly recommended but still optional. The only thing that isn't optional is delivering a working product.

Best practices == great, dogma == not great.

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

#118

Post by vonholstein » Mon, Oct 29, 2018, 1:32 PM UTC

vfc1 wrote: post #116

sethammons wrote: post #111

Earlier quoted context omitted.

For us, a failing unit test is almost always a bug. Lots of people write terrible unit tests. Too much mocking, asserting methods called, etc. Brittle. Test the edge of your public interface. Supporting methods only need specific unit tests if they are sufficiently complex. If your unit's interface changes or it's behavior changes, it should fail. That is a good thing.

think there is a lot of religious-like thinking when it comes down to tests: many people swear by them even against all the day to day proof to the contrary that they are really not that useful at the application level. I think if we are shipping a framework to thousands of users its great that it has unit tests, but if its an in-house application built by a team of 10 developers, I doubt that the tests are doing any…

Do you extend that opinion to both unit and integration tests? I agree with you that unit tests at the application level have low ROI, but I've found integration tests to be invaluable for applications/services, especially when performing large scale refactoring. Where I work currently there is a fairly large service, full of unit tests, but which the team is wary of changing because it lacks integration tests.

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

#119

Post by oldboyFX » Mon, Oct 29, 2018, 1:37 PM UTC

Folks, keep in mind that the value of code reviews, tests, etc. varies wildly from project to project.

- What kind of product are you building?

- How large is the project?

- How many developers are working on it?

- How experienced are the developers?

- How is the project being managed?

- Are there any hard deadlines?

- What is the budget? Burn-rate? Runway?

- Is the product established or are you just building the MVP and trying to validate market demand?

A two-person development team building an MVP with ever-changing requirements won't have the same workflow as an established company with a large development team and a bunch of existing users.

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

#120

Post by Telichkin » Mon, Oct 29, 2018, 1:51 PM UTC

carlsborg wrote: post #74

The core message is not "Code reviews are a bad thing" but "Blocked on code review is expensive" and I agree that aspect can be an issue: it can be a badly timed context switch for the reviewer, and we know context switches are expensive in terms of productivity, so in some cases it makes good sense to defer it to a later point in time. And this is valuable time lost for the author. So whats a better approach?

Have you tried a pair programming? I realized that this practice much more effective than code review. Pair programming encourages learning inside a team, interactions and code quality. It also reduces time to market.