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

111–120 of 150 posts

Re: Code reviews aren’t just for catching bugs

#111

Post by voidr »

Code reviews:

* signal distrust by default - the work I do is not to be trusted to be merged in and by extension, I'm not to be trusted

* can create a culture of passive aggressiveness - you do something that I don't like, I'll get back at you when I'll review your code

* not guaranty code quality - having a junior review a junior's code will not yield expert level code

Just because Google does code reviews doesn't mean you should, for Google a bug could cost millions, for your project a bug might cost 10$, however the overhead of code reviews might cost more than 100$. It's important to do numbers and think rationally.

Re: Code reviews aren’t just for catching bugs

#112

Post by Sacho »

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…

I agree that those things could be issues with code reviews, but I don't see how "promiscuous pairing" helps with most of them.

* They delay integration.

Pairing by definition slows down all work on code(you have 1 person working instead of two). Also, code reviews need not delay integration, even if you're following the article's suggestions. You can still merge all work in an integration branch, and pull it if it eventually doesn't pass code review.

* They tend to encourage focus on abstract code polishing without proportionality or relation to the value of the code.

This is an issue with the priorities of the people doing the code review, and thus applies just as much to pairing.

* They favor superficial improvements, while increasing the costs sunk into paths that may be deeply flawed. (They facilitate late code-structural feedback, but not early directional/problem-analytical feedback.)

I have no idea how pairing is possibly different from code review in this case.

* They often discourage more collaborative work and therefore quicker and richer feedback, by their presence as a substitute for pairing.

This is possibly true, so I'll just take the assertion at face value.

* They create impediments to work moving quickly to completion.

This is pretty much the same as #1.

"Of these, the most egregious is the distraction from value." - so the most egregious problem with code reviews is the people that do them might have wrong priorities. I don't see how pairing would change this.

A benefit of pairing that I can see over code reviews is when you get feedback - instant, real-time in pairing vs late in code reviews. The useful scenario I envision is when you start implementing a feature and your pair-buddy steers you away from dumping time into a poor solution. That and #4 seem to be significant reasons why you might want to introduce pair programming. However, pair programming is not without drawbacks when compared to code reviews - it takes up more time in most cases(this is where it derives its main benefit), some people work better solo, a mismatch of skill or expertise means one person has to slow down to work with the other. I wouldn't really say code reviews are "far inferior in every respect" to pairing, I think both have their place.

Re: Code reviews aren’t just for catching bugs

#113

Post by onion2k »

voidr wrote: post #111

Code reviews: * signal distrust by default - the work I do is not to be trusted to be merged in and by extension, I'm not to be trusted * can create a culture of passive aggressiveness - you do something that I don't like, I'll get back at you when I'll review your code * not guaranty code quality - having a junior review a junior's code will not yield expert level code Just because Google does code reviews doesn't m…

signal distrust by default - the work I do is not to be trusted to be merged in and by extension, I'm not to be trusted

That doesn't have to be a bad thing. You (and everyone else) can't be trusted to be totally infallible, so if you want to produce good work relying on many eyes to catch mistakes, suggest improvements, or to learn from one another then you need to look at everyone's code. A code review should be a conducted in a safe, blame-free environment where everyone involved wants to make better software. That's should be the goal, not finger pointing or points scoring.

Re: Code reviews aren’t just for catching bugs

#114

Post by reacweb »

voidr wrote: post #111

Code reviews: * signal distrust by default - the work I do is not to be trusted to be merged in and by extension, I'm not to be trusted * can create a culture of passive aggressiveness - you do something that I don't like, I'll get back at you when I'll review your code * not guaranty code quality - having a junior review a junior's code will not yield expert level code Just because Google does code reviews doesn't m…

My boss performs code reviews before my work is delivered. In my case it is mostly in a positive way: - he wants to know how stuff is implemented because he may need to maintain it if I am on leave. - it is a motivation for me to produce good code because I know it will be read - he may give good advice (for example usage of ArrayList instead of Vector in java)

Re: Code reviews aren’t just for catching bugs

#115

Post by aavz »

voidr wrote: post #111

Code reviews: * signal distrust by default - the work I do is not to be trusted to be merged in and by extension, I'm not to be trusted * can create a culture of passive aggressiveness - you do something that I don't like, I'll get back at you when I'll review your code * not guaranty code quality - having a junior review a junior's code will not yield expert level code Just because Google does code reviews doesn't m…

Assuming that people will make mistakes writing code isn't distrust, it's just common sense.

If someone is a good developer, I trust their first cut of code will be well thought out. I don't assume that they considered all corner cases or found the best design or written things in a way that makes sense to other people on the first try.

Things go downhill if people start taking code reviews personally, but then I don't think the real problem is code reviews.

Re: Code reviews aren’t just for catching bugs

#116

Post by aavz »

gurkendoktor wrote: post #83
frandroid wrote: post #55

Earlier quoted context omitted.

> the potential downsides of excessive code review What would those be?

Large commits getting stuck in review forever, or until they have too many conflicts to be merged cleanly; small commits being over-reviewed for trivial issues because the reviewer wants to prove that they have actually looked at the code.

On the other hand, if you skip code review for major or complicated changes, you end up with large chunks of code that only one person understands.

Sometimes that works out fine, but other times you don't find out until too late that there are major flaws.

Re: Code reviews aren’t just for catching bugs

#117

Post by aavz »

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…

I don't really see what your peer did wrong. Sometimes you need to see the actual implementation to understand how different approaches work out. There are definitely plenty of times I've started implementing A, then realised it doesn't work out that well.

Re: Code reviews aren’t just for catching bugs

#118

Post by lisivka »

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…

If you see a problem, then describe the problem and reject request.

Re: Code reviews aren’t just for catching bugs

#119

Post by profmonocle »

onion2k wrote: post #113
voidr wrote: post #111

Code reviews: * signal distrust by default - the work I do is not to be trusted to be merged in and by extension, I'm not to be trusted * can create a culture of passive aggressiveness - you do something that I don't like, I'll get back at you when I'll review your code * not guaranty code quality - having a junior review a junior's code will not yield expert level code Just because Google does code reviews doesn't m…

signal distrust by default - the work I do is not to be trusted to be merged in and by extension, I'm not to be trusted That doesn't have to be a bad thing. You (and everyone else) can't be trusted to be totally infallible, so if you want to produce good work relying on many eyes to catch mistakes, suggest improvements, or to learn from one another then you need to look at everyone's code. A code review should be a c…

It's not just programming. Journalists and authors have editors for this exact reason.

Re: Code reviews aren’t just for catching bugs

#120

Post by qb45 »

geophile wrote: post #19
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…

Amen. Another huge cost of code reviews is distraction. We've all seen the Paul Graham essay on maker's schedules vs. manager's schedules. We've all read the statistics on how much time is lost to interruptions. Code reviews are a massive interruption, done on a manager's schedule. Each code review is a distraction, and can take a significant time commitment, if it is to be a meaningful review. A few years ago, I wor…

> Finally, code reviews sometimes substitute for design review, which catches the most serious problems much earlier. At this same company, it drove me crazy that we always had time for code reviews, but never for design reviews.

You reminded me of one cargo-scrum team working like that.

Important design decisions were made thoughtlessly on "sprint planning" meetings since they were required for division of the "story" into "tasks". Then the classic "garbage in, garbage out" law guided the sprint.

Post reply on HN