Live data from Hacker News

Code reviews aren’t just for catching bugs

blog.fullstory.com

51–60 of 150 posts

Re: Code reviews aren’t just for catching bugs

#51

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.

I really like continuous integration/automated tests/code coverage dashboards/automated lint checkers, etc. The more decisions you can cleanly take away, the easier it is to work in a codebase. I agree with you there.

Once you have all that, the purpose of a code review changes a bit, but it is still really useful. Maybe you don't need locking, or maybe you should rewrite the base class, or maybe there is some other part of the code base that should be folded in, or maybe a better interface/algorithm. Stuff like that is hard for a computer to detect for you, at least for the next decade or two :).

The really big thing I push in code reviews is defensive programming. I want to make sure someone in a 2 years, who isn't familiar with the code, will not screw up the codebase after the original authors may have moved on to other things.

Re: Code reviews aren’t just for catching bugs

#52
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…

What? These statements and conclusions make no sense to me.

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.

You're using a terrible code review system. You should be able to have multiple potential commits out for review at any given time. You should also have multiple pending commits from your coworkers on your queue at any given time. The code review process is asyncrhonous: process them at your leisure (though sooner is better than later, for obvious reasons). Once the review has been approved, it can be rebased onto or merged into master. If the patches you're writing are so far-flung that they don't apply cleanly, break them into smaller pieces.

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

These two statements are contradictory. Monolithic commits by definition increase the diff with the codebase, not decrease it. What you tend to see are meaningful commits: commits which do one specific thing, and fully test it, without breaking any other systems.

Re: Code reviews aren’t just for catching bugs

#53
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…

That sounds like a culture problem. You don't really want people who are out to prove how smart they are, versus people that are trying to help the other engineers they work with get better.

Those two things can be presented as the same thing

Re: Code reviews aren’t just for catching bugs

#54

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…

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

Wait why are we doing code reviews on the manager's schedule? Why not just wait until you're finished a task to do them?

Re: Code reviews aren’t just for catching bugs

#55
post #2

I'd still rather earlier code reviews... design reviews about 1/3 of the way into writing the code. Enough time to have passed to have discovered the dragons in the whiteboard design, but not enough to have written code that could only undergo minor fixes in a code review. I prefer the idea of a code review happening at a time that it could still steer the ship... too many code reviews catch bugs, but don't correct p…

I think it will always depend on the team. Process X might be a savior to team Y, but completely screw team A even if team A == team Y (javascript truthiness here). On the flip side I think you can also say the potential downsides of excessive code review are significantly less then the potential downsides of zero code review. I dunno.

> the potential downsides of excessive code review

What would those be?

Re: Code reviews aren’t just for catching bugs

#56

Maybe I'm an arrogant XP-ist, but to me this sounds like a good step on the way to pair programming.

Depending on the org structure, pair programming can sometimes be a clearly superior version of a code review policy. Particularly when the review latency is high and, as a consequence, developers work on several branches in parallel.

Re: Code reviews aren’t just for catching bugs

#57
post #2

I'd still rather earlier code reviews... design reviews about 1/3 of the way into writing the code. Enough time to have passed to have discovered the dragons in the whiteboard design, but not enough to have written code that could only undergo minor fixes in a code review. I prefer the idea of a code review happening at a time that it could still steer the ship... too many code reviews catch bugs, but don't correct p…

One way to enforce this is to use source control for design docs, so that you can use the same pull request workflow as you do with code. We've done this by having user stories written in gherkin, so that new user stories can pass review, and then later can be implemented as tests. This can probably be extended to other types of design documentation.

Re: Code reviews aren’t just for catching bugs

#58

Earlier quoted context omitted.

I think it will always depend on the team. Process X might be a savior to team Y, but completely screw team A even if team A == team Y (javascript truthiness here). On the flip side I think you can also say the potential downsides of excessive code review are significantly less then the potential downsides of zero code review. I dunno.

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

Imagine your team prototyping game design ideas with average speed of 1-2 days per concept.

Re: Code reviews aren’t just for catching bugs

#59

Earlier quoted context omitted.

I think it will always depend on the team. Process X might be a savior to team Y, but completely screw team A even if team A == team Y (javascript truthiness here). On the flip side I think you can also say the potential downsides of excessive code review are significantly less then the potential downsides of zero code review. I dunno.

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

I'm not sure what ben_jones had in mind, but we've been doing a lot of pairing and mob programming where I work. I really enjoy it and it feels like the code review is basically just being done at all times (the reviewer is sitting right next to you as you write). But others feel like it eliminates some of the advantages of parallelism (i.e. 2 people working on 2 different things). It also slows down the amount of time to complete stories if one developer is less skilled than the other, but that issue should fade with time.

Re: Code reviews aren’t just for catching bugs

#60
post #4

I think a benefit not mentioned in the article is that it makes sure that at least one person besides the original author understands what the code does.

It's very important, and also it makes sure everyone is aligned about how to evolve the codebase, the project is in some coherent state and you can reason about the code as a whole (same patterns over the whole codebase etc.).

Also with code reviews you make sure other devs are not (re)introducing some bugs that have been fixed before (maybe in other places of the app), and so on. There can be objective technical bugs, but code review will catch also the misconceptions about how stuff is supposed to work in the app, which can have profound consequences - no matter how great your code is technically, it you're building a square while circle was requested, it's not good.

I'm absolutely stunned by the comments in this thread. I've worked for a short while in a team with no code reviews, everyone was pushing whatever the hell they wanted and it was a nightmare (and git history was a total spaghetti). Soon after I joined the pull request workflow was introduced but it was too late, the code was so bad it was unmanagable.

Unless you have really magnificent team, chance are too high that someone will be writing non-maintainable/non-debuggable code, using super confusing variables/methods/classes names, or reinventing the wheel, or not handling errors properly. "We don't do code reviews" is a no-go for me at this point, second to only "We use ClearCase for version control".

Post reply on HN