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

101–110 of 150 posts

Re: Code reviews aren’t just for catching bugs

#101

Post by r-s »

kerkeslager wrote: post #70
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 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. I'm on a team where for about a year, part of our process has been that code…

I have a hard time imagining that most code reviews take * Understanding of the problem at hand, reading through the story, understanding the context of the the code change.

* Pulling down the code, reading through the commit log

* Reading the specs, possibly running coverage tools if not part of testing suite.

* Understanding the logic of the code, seeing if the tests cover the edge cases

* If dependencies change, they may need to be investigated too

* Thinking if there are better ways make code a bit more extensible or understandable

* Thinking of risk of the code (security, performance, deployment concerns)

* Reading through associated documentation and ensuring its accuracy

Doing all of this takes me longer then 10 minutes always.

Re: Code reviews aren’t just for catching bugs

#102

Post by chris_va »

millstone wrote: post #85
chris_va wrote: post #81

Earlier quoted context omitted.

This can happen if code reviews are not done well, I agree. Things can be just as bad if code is not written well. Doing either well requires good engineering practices and skill. It is part of the profession that you acquire over time, like anything else that can improve development. Now, sometimes you are at the mercy of a system not filled with really experienced engineers. In such a system, though, I think skippi…

I was speaking more about the team organization. Code reviews increase in cost as the team becomes more heterogenous and more distributed. If your team consists of a kernel engineer in Paris and an Angular developer in San Francisco, code reviews will be high cost and low value no matter how experienced the devs are. I agree with you, even in the ideal case of a co-located homogeneous team, it's possible to screw up…

Completely agree.

Re: Code reviews aren’t just for catching bugs

#103

Post by timmy-turner »

jakub_g wrote: post #68
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.

Honest question: what strategy would you recommend to deal with a (senior) developer who specializes in opening gigantic pull request with significant number of bugs? (I invest a lot of time to read through the code and I catch lot of stuff, but it's draining huge amounts of my energy). Declining anything with coverage below 100% is not a viable option unfortunately, I think, and preaching about best practices gives…

I'd probably make your work visible by counting the hours spent in the review each day or week. If you are able to roughly calculate the costs associated with those reviews, you may be able to push your organization towards adopting better coding or testing standards. Alternatively, instead of reviewing the code and pointing out the bugs, suggest or write a few tests which will uncover those. Showing good test coverage in action may be better than just preaching it.

Re: Code reviews aren’t just for catching bugs

#104

Post by db1 »

wandernotlost wrote: post #87

While I value many of the same things as the author, I've found code reviews to be far inferior in every respect to pairing (especially promiscuous pairing (google it)), and to have negative effects in several important ways: * They delay integration. * They tend to encourage focus on abstract code polishing without proportionality or relation to the value of the code. * They favor superficial improvements, while inc…

Interesting, this is the first time I'm hearing about promiscuous pairing. How big the team at your company is and what percentage of time is spent pair programming?

Re: Code reviews aren’t just for catching bugs

#105

Post by hangonhn »

jakub_g wrote: post #68
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.

Honest question: what strategy would you recommend to deal with a (senior) developer who specializes in opening gigantic pull request with significant number of bugs? (I invest a lot of time to read through the code and I catch lot of stuff, but it's draining huge amounts of my energy). Declining anything with coverage below 100% is not a viable option unfortunately, I think, and preaching about best practices gives…

One of the really neat side effects of unit testing with high coverage is that it means your components must be suitable for more than one client: your actual product and the test cases.

This means it decouples all the components which leads to check-ins or pull requests that are also self contained and modular.

If you push for good test coverage (less than 100% but more than 80% perhaps?) for your unit tests, you'll get less bugs and more modular code and smaller check-ins.

Re: Code reviews aren’t just for catching bugs

#106

Post by skybrian »

r-s wrote: post #101
kerkeslager wrote: post #70

Earlier quoted context omitted.

> 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. I'm on a team where for about a year, part of our process has been that code…

I have a hard time imagining that most code reviews take * Understanding of the problem at hand, reading through the story, understanding the context of the the code change. * Pulling down the code, reading through the commit log * Reading the specs, possibly running coverage tools if not part of testing suite. * Understanding the logic of the code, seeing if the tests cover the edge cases * If dependencies change, t…

That sounds like a more thorough review than we're talking about here. It's more like following a link to a website, reading the diffs, and adding inline comments. And it's typically done by someone already somewhat familiar with the codebase.

Also, some code reviews are tiny changes, so it brings the average down.

Re: Code reviews aren’t just for catching bugs

#108

Post by machinshin_ »

dmarg wrote: post #23
eric_h wrote: post #6

Earlier quoted context omitted.

I work on a very small team (4 engineers), so we have all commits for all branches posted to our engineering chatroom. Everyone knows what everyone else is working on (and we all work in the same room), so when someone is on a task that we know my have some snags/difficult-to-design solutions, we'll all periodically take a break and look at decisions other people are making on their particular feature, providing feed…

I work on a team with the same size as yours. Although we do not all sit in the same room, we still do code reviews. Since we are small we are not the most critical on style. I am a big believer in code reviews especially starting early with a small team. This would set the culture from the beginning because it is harder to bring that in later.

imo, when it comes to style, if your language ecosystem has one, you should use it to enforce one. ideally as a git pre-commit hook.

Re: Code reviews aren’t just for catching bugs

#109

Post by solipsism »

wellpast wrote: post #45
renku wrote: post #39

Earlier quoted context omitted.

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 re…

I'm curious what you mean by grab a lock and schedule and wait for a code review. I've used all sorts of different SCMs, code review software, in-person code review processes, etc.. but I've never heard of anything like that. Could you describe your process to us in more detail?

Re: Code reviews aren’t just for catching bugs

#110

Post by nvarsj »

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 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].

I don't get why you think code reviews are so expensive. An engineer should rarely be blocked by a code review. If waiting for a review - don't. Pick up another task! A great thing about code reviews is they are done async.

If the cost is time spent doing the review - what's the alternative? No code review at all? That's like not testing - yes it's faster now, but you'll pay for it dearly later on. (Code review is well studied in academia / industry, and has consistently shown to be very effective. Having at least one extra pair of eyeballs on any code goes a long way.)

Post reply on HN