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

Code reviews aren’t just for catching bugs

blog.fullstory.com

Post reply on HN
Print view View original

41–50 of 150 posts

Re: Code reviews aren’t just for catching bugs

#41

Post by UK-AL »

wellpast wrote: post #33
chris_va wrote: post #17

Earlier quoted context omitted.

Why do you find them expensive? I've done thousands in my career. They don't take much time compared to actually writing the code, and adding an extra 5% of engineering time pays major dividends later without drastically reducing throughput.

For me continuous integration and refactoring have been the most important practices in keeping a code base clean, robust and agile. Code review cultures tend to encourage the opposite of these agile practices: monolithic commits, infrequent integration, and minimal diffs -- in other words, practices that tend to result in lesser productivity.

How hell does code review encourage those behaviours?

The best commits for code review are small ones. Nobody can be bothered with the big ones.

Re: Code reviews aren’t just for catching bugs

#42

Post by eduren »

collyw wrote: post #22
eduren wrote: post #12

Working at a Gov't IT contractor, I really wish we had the organizational skills/incentive to do code reviews. Very rarely does the code I write get glanced at, much less examined. My instinct, of course, is to solicit code reviews from my peers, but the organizational structure and support tooling are all woefully inadequate. Plenty of projects, even greenfield ones, aren't checked into source control. And of course…

Hardly any one gives a shit about code quality, except the developers that have to maintain it afterwards. Saying that, poor code quality has been a real incentive for me to improve my own code. When I eventually work out what something is doing and see how much simpler it could be i try to redo it in a way that will be a lot more understandable to the next person. If I visit my own code a month or two later and don'…

True, code quality isn't as important as some people think. But I think that's there's different levels to indifference that can cause serious problems.

Code quality that simply reduces bugs and speeds maintenance is a lot less important than the kind of code quality that creates architecturally sound solutions and good products. Where I work, the lack of quality control/assurance affects so many things and has a visible detriment to the products.

Re: Code reviews aren’t just for catching bugs

#44

Post by fishnchips »

draw_down wrote: post #25
fishnchips wrote: post #5

Earlier quoted context omitted.

This is the role of design documents which get heavily reviewed by the team before even the first line of code is written.

We write those on my team. They usually end up only faintly resembling what actually gets shipped. No matter how much agreement there is beforehand, as soon as people see working software they change their minds. The best thing is to plan for and expect change, not try to mitigate its manifesting.

True, if your software is well designed and modular then it's easy to contain necessary changes to a single subsystem. But a good design document would allow you just that - designate boundaries and interfaces between subsystems. I personally found out that writing an IDL (Protocol Buffers, Thrift or what have you) along your design document is very useful since it nicely bridges abstract concepts with actual code.

Re: Code reviews aren’t just for catching bugs

#45

Post by wellpast »

renku wrote: post #39
wellpast wrote: post #11

What often goes unmentioned in praise for code review processes is their insanely exorbitant costs -- measured in engineer hours but perhaps more costly is all of the blocking and impedance [1]. Most of the "problems" that code reviews claim to address can be solved by much more direct and optimal measures. Code reviews are damn expensive. This post concedes that code reviews are better for the more fluffy ends -- te…

What are these "more direct and optimal measures" that are cheaper than code reviews? In my experience code reviews are much cheaper than other means of raising software quality. For example unit tests only start to add real value after you have written a good bunch of them, so they form this regression-safety-net. I haven't seen Continuous integration and refactoring being thrown out of the window because of code re…

Continuous integration means you can work on multiple items at once and that you break up large feature dev into smaller chunks that get committed to master. For example, I might commit the data layer for a new big feature well before the UX, etc.

Code reviews mean that every time I want to integrate with master I've got to 'grab a lock' and schedule and wait for a code review.

What typically happens in these code review cultures is you tend to make monolithic commits (to minimize the # of code reviews you have to do) and you also tend to minimize diffs with the code base (to make the CR easier for the reviewer).

Re: Code reviews aren’t just for catching bugs

#46

Post by pmarreck »

hangonhn wrote: post #7

I would go further and say that you should never trust code reviews to catch bugs. They're great for all the reasons in the article and what "zhemao" said but they're horrible for catching any bugs beyond the most trivial ones. Humans simply aren't good at running code in their head like that. Code review is no substitute for good code coverage and automated testing, preferably pre-commit.

I've personally (YMMV) found that going all the way to full TDD has resulted in the fewest delivery bugs (by a lot). Beats out unit testing for sure, but either of those are miles ahead of NO testing.

Re: Code reviews aren’t just for catching bugs

#47

Post by phasmantistes »

draw_down wrote: post #25
fishnchips wrote: post #5

Earlier quoted context omitted.

This is the role of design documents which get heavily reviewed by the team before even the first line of code is written.

We write those on my team. They usually end up only faintly resembling what actually gets shipped. No matter how much agreement there is beforehand, as soon as people see working software they change their minds. The best thing is to plan for and expect change, not try to mitigate its manifesting.

Yep, and design docs are the most important part of that planning for change. If you haven't planned the desired design, you can't see how circumstances are forcing you to change it, and what tradeoffs you'll be making to accommodate those circumstances.

Re: Code reviews aren’t just for catching bugs

#48

Post by [deleted] »

hangonhn wrote: post #7

I would go further and say that you should never trust code reviews to catch bugs. They're great for all the reasons in the article and what "zhemao" said but they're horrible for catching any bugs beyond the most trivial ones. Humans simply aren't good at running code in their head like that. Code review is no substitute for good code coverage and automated testing, preferably pre-commit.

[deleted]

Re: Code reviews aren’t just for catching bugs

#49

Post by wellpast »

CorvusCrypto wrote: post #35
wellpast wrote: post #11

What often goes unmentioned in praise for code review processes is their insanely exorbitant costs -- measured in engineer hours but perhaps more costly is all of the blocking and impedance [1]. Most of the "problems" that code reviews claim to address can be solved by much more direct and optimal measures. Code reviews are damn expensive. This post concedes that code reviews are better for the more fluffy ends -- te…

I feel like this can be argued so many ways. For instance, while code review might block some changes from being introduced into the system, it also does have more benefits than the "fluffy" ones you mentioned. Maybe an engineer that has more experience with the codebase can point out a way of doing something that is more in line with what the team's style is (earlier move toward standardized code vs letting people d…

So the problem you're talking about is how to properly manage (and coach) less experienced devs or devs who write messy code. While code reviews can tangentially deal with this, I think there are much more direct approaches to these problems than an umbrella code review policy.

Re: Code reviews aren’t just for catching bugs

#50

Post by Domenic_S »

msoad wrote: post #20

Code reviews can easily become a tool for people with huge egos to prove their smartness. I get code review comments for grammar of my comments or very small code style preferences that Google's anal style guide can't enforce (yet). I like code reviews, don't get me wrong. But there should be a way to respond with "you just shut up, you're only trying to make yourself look smart". It's all because higher up people mo…

There's a line to walk for sure. When I code review a line that looks like this:

  $foo=fn1($bar) &&$baz = fn2 ($qux)
I always send it back. Do I look pedantic? Probably, but to me consistency is important. It's about caring about what you're doing. If I see a line like this, I assume the person who wrote it doesn't pay a lot of attention to detail and I pay closer attention to the rest of their commit. I mean, if you can't be troubled to set your IDE preferences to the team/project's rules, what's that say about you?
Post reply on HN