Are they committing to their own branch? That may be your colleagues argument for post-commit review. Personally I would say pre-commit for very inexperienced developers.
–
Simon WhiteheadSep 7 '12 at 10:27

16 Answers
16

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.

If the developers have their own branches and you have a proper code-review tool, you can maintain control. Reviewers have to record in the tool whether or not they've done the review.
–
MarkJSep 7 '12 at 11:55

1

It should be added that having a commit reviewable implies that the coder himself have a much clearer mind, enforcing the fact that each issue must be dealt separately in small successful steps. It tightens feedback loops and seems a must for any agile team.
–
vaabSep 7 '12 at 13:05

3

+1, this works even better when each developer doesn't have their own branch, but when you use feature branches instead. We commit bugfixes directly to trunk, since they're usually small, but features go onto their own branch, get many commits, then can be reviewed before being merged to trunk.
–
IzkataSep 7 '12 at 16:27

1

@ThomasOwens: Perforce supports branching, but not with the ease of SVN, GIT, or Mercurial.
–
kevin clineSep 14 '12 at 19:34

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.

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.

This suggests that minor problems or suggestions are either "fixed" by the reviewer, or not at all? I would expect that ANY review comments be fed back to the author to address (or reject)
–
AndrewSep 8 '12 at 8:18

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.

Pre-commit code reviews seem so natural, it never occurred to me that reviews could deliberately be done after committing. From a continuous integration perspective, you want to commit your code once it is finished, not in a working-but-possibly-poorly-written state, right ?

Maybe it's because the way we've always done it in my teams is live dialog initiated by the original developer, not asynchronous, control-driven, document-based reviews though.

A system like gerrit or clover (I think) can stage a change and then have the reviewer commit it to the source control (push in git) if it's good. That's the best of both world.

If that's not practical, I think that after commit is the best compromise. If the design is good then only the most junior developers should have things bad enough you don't want them committed ever. (Make a pre-commit review for them).

Which leads to design review - while you can do it at code review time (or for that matter at customer deployment time), finding design issues should be done earlier than that - before the code is actually written.

With peer review there is a minimal risk of losing control. All the time two people have knowledge about the same code. They have to switch occasionally, so they have to be attentive all the time to keep track of the code.

It makes sense to have a skillful developer and a newbie working together. At first glance this seems to be inefficient, but it isn't. In fact, there are fewer bugs, and it takes less time to fix those. Besides, the newbies will learn much faster.

What comes to preventing bad design, this should be done before coding. If there is any considerable change/improvement/new piece of design, it should be reviewed before coding starts. When design is completely developed, there is not much left to do. Reviewing the code will be easier and will take less time.

I agree that it is not essential to review code before committing only if the code is produced by an experienced developer, who have already proved their skills. But if there's a newbie, code should be reviewed before committing: the reviewer should sit next to the developer and explain every change or improvement made by them.

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

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

It depends on your team make up. For a relatively experienced team that is good about small, frequent commits then post-commit review just to get a second pair of eyes on the code is fine. For larger, more complex commits and/or for less experienced developers then pre-commit reviews to fix problems before they get in seems more prudent.

Along those lines, having a good CI process and/or gated check-ins lessens the need for reviews before commit (and arguably post commit for many of them).

I would recommend adjusting your branching strategy. Using a developer branch or feature branch has a number of benefits... not the least of which is facilitating post-commit code reviews.

A tool like Crucible will smooth and automate the review process. You can select one or more committed changesets to include in the review. Crucible it will show which files were touched in the selected changesets, keep track of which files each reviewer has already read (showing a % complete overall) and let the reviewers easily make comments.

100% paired programming (no matter how senior you think you are) with lots of small commits and a CI system that builds on EVERY commit (with automated testing including units, integration and functional wherever possible). Post-commit reviews for large or risky changes. If you must have some kind of gated/pre-commit review, Gerrit works.

You should be reviewing your own code summarily before committing it. In Git, I think the staging area is great. After I've staged my changes, I run git diff --cached to see everything that is staged. I use this as an opportunity to make sure I'm not checking in any files that don't belong (build artifacts, logs, etc.) and making sure that I don't have any debug code in there or any important code commented out. (If I'm doing something that I know I don't want to check in, I usually leave a comment in all caps so that I'll recognize it during staging.)

Having said that, your peer code review should be generally be conducted after you commit, assuming that you are working on a topic branch. This is the easiest way to make sure that everybody else is reviewing the correct thing, and if there are major problems, then it's no big deal to fix them on your branch or delete them and start over. If you conduct code reviews asynchronously (i.e. using Google Code or Atlassian Crucible), then you can easily switch branches and work on something else without have to separately keep track of all your different patches/diffs that are currently under review for a few days.

If you're not working on a topic branch, you should. It reduces stress and hassle and makes release planning much less stressful and complicated.

Edit: I should also add that you should be doing code review after testing, which is another argument in favor of committing code first. You don't want your test group fiddling around with dozens of patches/diffs from all the programmers, then filing bugs just because they applied the wrong patch at the wrong place.

we actually do a hybrid on LedgerSMB. Committers commit changes that get reviewed after. Non-committers submit changes to committers to be reviewed before. This tends to mean two tiers of review. First you get a mentor to review and mentor you. Then that mentor gets the code reviewed a second time after he or she's signed off on it and feedback circulates. New committers usually spend a lot of time reviewing other people's commits at first.

It works pretty well. The thing though is a review after is usually more cursory than a review before, so you want to make sure that review after is reserved for those who have proven themselves. But if you have a two tier review for the new folks it does mean that problems are more likely to be caught and discussions had.

Just don't do code reviews at all. Either you believe your developers are capable of writing good code, or you should get rid of them. Errors in logic should be caught by your automated tests. Errors is style should be caught by lint and static analysis tools.

Having humans involved in what should be automatic processes is just wasteful.