Traditionally we performed code review before commit, and I had an argument with my colleague today, who preferred code review after commit.

First, here's some background,

1. We have some experienced developers and we also have new hires with almost zero programming experience.
2. We'd like to perform fast and short iterations to release our product.
3. All team members are located at the same site.

Answer: What's Your Branching Strategy? (37 Votes)

If the developers have their own private branch for development (which I'd recommend in most situations anyway), I'd perform the code review prior to merging with the trunk or main repository. This will allow developers to have the freedom to check in as frequently as they want during their development/testing cycle, but any time code goes into the branch that contains the delivered code, it gets reviewed.

Generally, your bad experiences with code reviews sound more like a problem with the review process that have solutions. By reviewing code in smaller, individual chunks, you can make sure it doesn't take too long. A good number is that 150 lines of code can be reviewed in an hour, but the rate will be slower for people unfamiliar with the programming language, the system under development, or the criticality of the system (a safety critical requires more time)—this information might be useful to improve efficiency and decide who participates in code reviews.

Answer: Pre-Commit (15 Votes)

I've recently started doing pre-commit reviews in a project I'm in and I must say I'm pleasantly surprised about how unproblematic it is.

The biggest drawback of post-commit reviews that I see is that it's often a single-person-only afair: Someone reviews the code for correctness, but the author is not usually involved unless there's a serious mistake. Small problems, suggestions or hints don't usually reach the author at all.

This also changes the perceived result of the code reviews: it's seen as something that only produces additional work, as opposed to something where everyone (the reviewer and the author of the code) can learn new things every time.

Answer: Check in Early & Often (7 Votes)

Developers who work for long periods—and by long I mean more than a day—without checking anything into source control are setting themselves up for some serious integration headaches down the line. Damon Poole concurs:

Developers often put off checking in. They put it off because they don't want to affect other people too early and they don't want to get blamed for breaking the build. But this leads to other problems such as losing work or not being able to go back to previous versions.

My rule of thumb is "check-in early and often", but with the caveat that you have access to private versioning. If a check-in is immediately visible to other users, then you run the risk of introducing immature changes and/or breaking the build.

I'd much rather have small fragments checked in periodically than to go long periods with no idea whatsoever what my coworkers are writing. As far as I'm concerned, if the code isn't checked into source control, it doesn't exist. I suppose this is yet another form of Don't Go Dark; the code is invisible until it exists in the repository in some form. ...If you learn to check in early and check in often, you'll have ample time for feedback, integration, and review along the way...

I've worked for a couple companies that had different approaches towards this. One allowed it, as long as it didn't prevent compiling. The other would freak out if you checked in any bugs at all. The former is much preferred. You ought to be developing in a kind of environment that would allow you to collaborate with other people on things that are still in progress, with the understanding that it will all be tested later.

The most direct way to improve as a software developer is to be absolutely fearless when it comes to changing your code. Developers who are afraid of broken code are developers who will never mature into professionals.

I would also add that for peer reviews, if someone wants you to change something, having the history of your original version in source is a very valuable learning tool.

Answer: Two-Phase Commit (5 Votes)

Most repositories today support a two-phase commit or a shelveset (private branch, pull request, patch submission or whatever you want to call it), that will allow you to inspect/review work before pulling it into the main line. I would say that leveraging these tools would allow you to always do pre-commit reviews.

Also, you might consider pair coding (senior pairs with junior) as another way of providing a built-in code review. Consider it as a quality inspection on the assembly line instead of after the car has rolled off.

Answer: Weigh Benefits, Decide (1 Vote)

Gives reviewers confidence they are reviewing the author's latest revision.

Helps insure everyone reviews the same code.

Gives a reference for comparison once revisions made from review items are complete.

No Running Commits During the Review

I have used Atlassian tools and have seen running commits occur during the review. This is confusing to reviewers, so I recommend against it.

Post Review Revisions

After reviewers complete their feedback, verbally or in writing, the moderator should ensure the author makes the requested changes. Sometimes reviewers or the author may disagree as to whether to designate a review item as a fault, suggestion, or an investigation. To resolve disagreements and ensure investigation items are cleared correctly, the review team depends on moderator judgment.

My experience with around 100 code inspections is that when reviewers can reference an unambiguous coding standard, and for most kinds of logic and other programming faults, review results are generally clear cut. Occasionally there is a debate about nit-picking or a point of style can degenerate to argument. However, giving decision power to the moderator avoids stalemate.

Post-Review Commit

Gives the moderator and other reviewers a data point to compare against the pre-review commit.

Provides metrics for to judge both the value and success of the review at defect removal and code improvement.