No matter how you review code, you have to answer this basic question:
I find most people come down strongly on one side or another; and since
we make a code review tool, this is a point of contention for a lot of
our customers. Here's some things to consider when you're making the decision:
Pre-commit review blocks developers
As soon as the review is created, the developer is now "frozen." If a defect appears in the review she'll have to edit the old files exactly as they were; that means she can't continue working on those files, and she'll have to work hard to track any additional files she starts working on.
So what does she do? Answer email? Watch a Susan Boyle video?
This penalty can be addressed in a few different ways:
- Use multiple workspaces/checkouts so you can just switch to another and continue working.
- Use "local sandbox" branches so it's easy to switch between tasks. In this case you probably want to always commit to the local branch and pre-commit-review the branch-integration step.
- Use a DVCS that makes local sandboxes and repositories and branches trivially easy.
- Some IDEs allow you to "save" your current file state and content, revert to another state, and switch back later. That's good, but make sure that when you "unfreeze" the original state you don't blast the fixes you just made! That is, "unfreeze" needs to be "merge the saved diffs with the current content" and not "replace current content completely with what I previously saved."
- Mandate that code reviews have higher priority than other tasks so they're done quicker.
Post-commit review makes it slower and harder to fix bugs
If the code review uncovers a few defects, what happens if the developer is already off merrily working on the next task? Now new files are checked out, and worse the developer might be doing additional work on the files under review!
This is unlike a pre-commit review where the files are already up in the IDE and the code is fresh in the mind of the developer. Fixes can sometimes take under a minute.
This penalty can be addressed in a manner similar to pre-commit blocking — branches and workspaces.
Another strategy you probably want to employ is to allow the developer to complete the current task, check that in, and then work on the defects found in code review. The problem is that often the code review fix also affects what you're currently working on — now you're making more work for yourself.
Pre-commit review is less workflow
With pre-commit, "checked in" also means "code reviewed." Otherwise you have to track what's been reviewed, what hasn't, and what are the open bugs.
With Code Collaborator it's not too hard — you can have post-commit reviews created automatically with a version control trigger and reports tell you the rest — but it's still more work, and without a tool it's a lot of work. With something like a pass-around-the-email process, it's impossible. (Example: What is an email-based review finished? When everyone shuts up?)
Post-commit review on branch-integration can be too big
If you just review branch-integrations (as suggested above), you run the risk of mammoth reviews. This is a problem both in number of files and changes to review and in number of concepts (i.e. features/bugs) being simultaneously reviewed.
If someone asked you to fix 10 bugs simultaneously, it would be much harder than fixing each one, one at a time. But that's what exactly happens when you dump a massive review into someone's lap!
Besides this common-sense argument against big reviews, there's hard data showing that far fewer defects are found in big reviews as when you break them up into smaller ones.
You can mitigate this problem by mandating that each branch-integration is a single "concept" (e.g. one feature or one bug-fix). That's easy if by "branch" you mean "local sandbox." It can also be easy with a DVCS and some scripting.
Pre-commit review protects others
Pre-commit review ensures that other developers in your group aren't affected by the bugs that the code review is about to uncover. This can save build-breaks or other crippling bugs. It only takes a few of these a month to blast a lot of developer time!
You can do both
During the current v5.0 beta period we've adopted a hybrid approach. If the change is tiny and the author thinks there's only a small chance of a defect, the code is committed right away and reviewed after. Otherwise we do our usual pre-commit review.
This way we can go fast on the little and (relatively) safe stuff.
It's OK to "trust" people to make this call, because the worst-case isn't that bad — you still have a review and you still have to fix the bugs, and the only person who "loses" in this case is the author who made the bad call.
I hope these considerations help you make the choice that's best for your development group.
What's your opinion on this issue? Do you have more pros/cons to add? Leave a comment!