Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

A bogus study on code review

blog.wesleyac.com

Post reply on HN
Print view View original

51–60 of 61 posts

Re: A bogus study on code review

#51

Post by gregmac »

Clubber wrote: post #6

I don't like code reviews, but it has a few benefits: Pros: 1. Developers pay more attention to what they throw over the wall as a completed tasks because they will get grilled on it. 2. Reviewing other people's code is good for learning a new system. 3. It keeps creating of methods that already exists to a minimum and boosts proper code reuse (of existing, difficult to find code). Cons: 1. It takes a lot of time. 2.…

It is a fallacy that it takes a lot of time, like saying QA is worthless because it too takes a lot of time. Just like it's easier to fix a bug when QA finds it before a customer does, it's even easier to fix when a developer spots it in code.

If you think you stop making coding mistakes after a few months (well, or ever), you're kidding yourself.

I've never got into an argument over code style in a PR, and it seems like if this did happen I'd be looking to fix deeper problems in the team. I've had comments in open source PRs asking me to change something, but that's part of getting into a new code base. The right style is there one that's there, so unless your PR has the sole purpose of specifically changing the style, you should comply with existing style, no arguing.

Anyway, there are lots of other pros:

A reviewer might suggest a better way of doing something, helping others learn, and likewise will pick up new techniques from the code they're reviewing. This applies to even the most senior, experienced person in the team.

If a new library is brought in, it can be checked for conflicting versions or overlap/redundancy with an existing library, and license compliance.

Noticing simple things like hardcoded parameters/constants, unchecked config or input values, spots with potential unhanded errors, among dozens of other problems.

Unrelated changes are usually caught. Even if it's fixing another bug, I'd argue it's better to do separately with its own issue, because otherwise it gets lost when you go to build release notes or a change log.

Re: A bogus study on code review

#52

Post by valuearb »

sharkdesk wrote: post #33
valuearb wrote: post #28

Earlier quoted context omitted.

I can’t imagine any reason to skip code reviews in any professional development. Someone is paying the team to produce professional code, finding out well after the fact that one section/contributed failed that test is unacceptable. Code reviews shouldn’t be lots of overhead. They shouldn’t devolve into style critiques. They should be the best tool you have for teaching and communicating best practices. Reviewing a m…

Actually, someone is paying the team to solve business problems and create revenue.

The people who pay my team mostly want bugs fixed. Bugs going in testing are far more expensive to fix than those found in code review.

Re: A bogus study on code review

#53

Post by valuearb »

watwut wrote: post #36
valuearb wrote: post #28

Earlier quoted context omitted.

I can’t imagine any reason to skip code reviews in any professional development. Someone is paying the team to produce professional code, finding out well after the fact that one section/contributed failed that test is unacceptable. Code reviews shouldn’t be lots of overhead. They shouldn’t devolve into style critiques. They should be the best tool you have for teaching and communicating best practices. Reviewing a m…

"They should be the best tool you have for teaching and communicating best practices. " For christ sake, no. If you want communicate new best practice, do so. And if team agrees, we will follow. Don't ambush me with new expectations during code review and don't keep my task hostage in order to push for your new best practice. Same with teaching. Teach proactively instead of pretending juniors can solve it and then re…

I have zero expectations on formatting preferences. But i have high expectations on writing correct, maintainable code. If i see code that can fail in ways you haven’t anticipated, or is a duplication of other code in the project i’m going to point that out. If you implement something in a way i wouldn’t, or can’t understand, it may be an opportunity for you to teach me something.

And if I tell you to re-implement working code my way simply because of some personal preference, i would expect you to tell me to fuck off. Cause that’s what i’d say if you did it to me.

To give you an idea of how our code reviews are perceived, we have one junior dev, at last retrospective she spoke up about how much she valued and appreciated them.

Obviously some places don’t get it, but i think we do.

Re: A bogus study on code review

#54

Post by Clubber »

gregmac wrote: post #51
Clubber wrote: post #6

I don't like code reviews, but it has a few benefits: Pros: 1. Developers pay more attention to what they throw over the wall as a completed tasks because they will get grilled on it. 2. Reviewing other people's code is good for learning a new system. 3. It keeps creating of methods that already exists to a minimum and boosts proper code reuse (of existing, difficult to find code). Cons: 1. It takes a lot of time. 2.…

It is a fallacy that it takes a lot of time, like saying QA is worthless because it too takes a lot of time. Just like it's easier to fix a bug when QA finds it before a customer does, it's even easier to fix when a developer spots it in code. If you think you stop making coding mistakes after a few months (well, or ever), you're kidding yourself. I've never got into an argument over code style in a PR, and it seems…

>It is a fallacy that it takes a lot of time, like saying QA is worthless because it too takes a lot of time.

If you think it's so valuable, why not have every developer in the team review every merge? That's like 15 code reviews for each piece of code checked in. Obviously, it's because at some point you get diminishing returns. We have developer testing, unit testing, QA and UAT, all to catch defects before production. That's 4 layers of testing. Code reviews make it 5 layers. Not only is it a fifth layer, it's expensive because the most expensive people (in both wages and opportunity cost) are doing it. Reviews are just another iteration of essentially the same thing. At some point, you are spending a lot of money on little return. For me that is mandatory code review after 3-6 months.

Re: A bogus study on code review

#55

Post by Clubber »

valuearb wrote: post #52
sharkdesk wrote: post #33

Earlier quoted context omitted.

Actually, someone is paying the team to solve business problems and create revenue.

The people who pay my team mostly want bugs fixed. Bugs going in testing are far more expensive to fix than those found in code review.

>Bugs going in testing are far more expensive to fix than those found in code review.

How so? Particularly if you don't do your code review right after development. That holds true for production vs. QA, but I'm not sure it does for QA vs. code review, unless your QA deployment and ticket bounce back process is overly cumbersome and you don't have time planned for it.

Re: A bogus study on code review

#56

Post by hacker314159 »

A lot of the comments here imply that style can be fixed with a linter, but what about the style of things other than syntax, like choices in variable names, DRY-ing through mixins vs composition, grammatical style in code comments, etc.

Re: A bogus study on code review

#57

Post by Anticapitalist »

Clubber wrote: post #37
sharkdesk wrote: post #33

Earlier quoted context omitted.

Actually, someone is paying the team to solve business problems and create revenue.

Preferably in the most efficient way possible (which is of course subjective). I believe that after 3-6 months in a particular subsystem, the ROI of code reviews increasingly decreases over time.

But shouldn't the time required to review also drop significantly when the reviewer is familiar with the module?

Re: A bogus study on code review

#58

Post by sanderjd »

sharkdesk wrote: post #47
sanderjd wrote: post #44

Earlier quoted context omitted.

Consistent style is useful because it reduces cognitive load when reading unfamiliar code, but everyone should have automated tools that do that "95%", and most people don't. This is something that Go got very right by building it straight into the compiler but automatic linter-formatters exist for most languages, and it's useful for someone high up in the early technical team to just pick some flags they like for on…

linters and auto formatters add a lot of value, but some style changes don't fall into that. e.g.: naming things, or just when the language provides more than one way to do something, which one you choose.

I don't consider either of those to be matters of "style". Naming is more akin to documentation, and as such, it is a very useful thing to review: if names don't make sense to a reviewer now, they're even less likely to make sense to someone reading them in the future.

Picking which among a few ways of doing something is idiom, not rote style. I agree that this is one of the least valuable parts of review, but people ramp up quickly on it in my experience, so it isn't constantly coming up. If an idiom actually matters, for instance if it is protecting against some commonly problematic pattern, it may be possible for tooling to catch that too. If it really doesn't matter which way its done, the reviewer should just let the author do it their way, and if they don't, the author should just change it without making a fuss. But yeah, arguing over idioms can be a problem. It helps to write down decisions the team has made in the past, so that you can just point dissenters to the documentation.

Re: A bogus study on code review

#59

Post by watwut »

valuearb wrote: post #53
watwut wrote: post #36

Earlier quoted context omitted.

"They should be the best tool you have for teaching and communicating best practices. " For christ sake, no. If you want communicate new best practice, do so. And if team agrees, we will follow. Don't ambush me with new expectations during code review and don't keep my task hostage in order to push for your new best practice. Same with teaching. Teach proactively instead of pretending juniors can solve it and then re…

I have zero expectations on formatting preferences. But i have high expectations on writing correct, maintainable code. If i see code that can fail in ways you haven’t anticipated, or is a duplication of other code in the project i’m going to point that out. If you implement something in a way i wouldn’t, or can’t understand, it may be an opportunity for you to teach me something. And if I tell you to re-implement wo…

I don't mind formatting requirements - when they are known in advance and same for everybody.

But last time there were two "seniors", each with different conflicting preferences and each insisting that your code is "bad" and "mess". (And inexperienced project manager buying their bs because they were confident). And it changed based on what blogs they read last week. Juniors were more fine with that then me, because they tended to belive seniors know what they are doing. Plus juniors worked only on one module and I did on two, so I could see how what one called crap on the meeting is demanded by the other.

And the rules were different for different people.

I think it is the ability of coder to tell "fuck off" on changes he disagree with that makes all the difference. I was comiting code I disagreed with and had to come Sunday to make changes I considered useless.

I used to be for code reviews, but now I think that it requires people with strong self awareness, knowing difference between preference/feeling and objectively betters, and many programers out there don't have that. The seniors I had trouble with were on spectrum, one a little bit other a lot. They could not tell the difference and it cause a lot of problems beyond code reviews.

Re: A bogus study on code review

#60

Post by valuearb »

Clubber wrote: post #55
valuearb wrote: post #52

Earlier quoted context omitted.

The people who pay my team mostly want bugs fixed. Bugs going in testing are far more expensive to fix than those found in code review.

>Bugs going in testing are far more expensive to fix than those found in code review. How so? Particularly if you don't do your code review right after development. That holds true for production vs. QA, but I'm not sure it does for QA vs. code review, unless your QA deployment and ticket bounce back process is overly cumbersome and you don't have time planned for it.

We always do code review before QA. But our QA process is also multilayered, first layer is QA engineer, next layer is first level business acceptance testing, then another higher level business acceptance testing. Gets pretty expensive once the SR marketing person rejects.

No i didn’t design this process.

Post reply on HN