Commit Early Commit Often With Code Reviews?

Question

Hi,

I'm looking for advice/experience/best-practices on how to get a team of developers engaged in a commit early commit often work paradigm while still benefitting from a thorough code review process. We currently use review board and it is policy that no code be committed to SCM w/o a peer review. While I completely embrace code reviews it appears to slow the overall process and presents a huge speed bump to the commit early commit often idea. So I'm asking for other's experience in what works and what doesn't. Do you commit early and often? Do you also use code reviews? Is there an obvious way (that I'm missing) where the two ideas can blend?

Answer 1

I would suggest maybe switching to distributed SCM. You could do so that every member on the team commits to it's own repository. When a feature is ready for review, the reviewer is notified. If the reviewer accepts the changes, he pushes/merges them in the stable repository (which is quite painless).

Git and Mercurial (hg) are good examples of distributed SCM's

Answer 2

First of all, what revision control software are you using?

Depending on your RCS software, you can achieve both goals at the same time. For example, if you are using Subversion, each developer can commit early and often to their own private development branch. Peer reviews can be required before any development branches are merged into the trunk. Other RCS systems can most likely do something similar.

Edit: I forgot to mention that if you are indeed using a "commit early and often" strategy with Subversion (with or without development branches), developers need to make sure that they update their working copy with the changes committed to the trunk equally as often. This was a problem my team had for a while after we migrated from VSS to Subversion. We were not accustomed to being able to do 3-way merges, and developers would update their working copy infrequently (sometimes less than twice a day) which would usually require a significant amount of manual merging. Eventually, everyone got accustomed to running a svn update whenever a commit is made (we have a post-commit hook that updates an RSS feed with new commits) or, at a minimum, every hour or so. As a result, most of the merging is handled automatically and the amount of manual work required to merge a development branch's changes back into the trunk is typically trivial. This also keeps the diff between the trunk and the development branch as small as possible, which makes code review easier (since we typically focus on the diff between branch and trunk when reviewing code).

Answer 3

Use a distributed version control system. One of the biggest drawbacks to centralized version control is that it hinders "commit early, commit often". Distributed systems encourage it.

Answer 4

Apply this method to all text-based files: break up your SO paragraph into small groups of sentences.

Answer 5

All of the existing answers propose distributed SCM. This is sufficient, but not necessary. Before the existence of Distributed SCM, it was still possible to do this.

Each developer should work in their own branch (or, you may prefer to lower levels of branch-granularity such as per bug or task (especially with pair programming, or developers who work on many tasks at once). This decision can probably be left to the developer, if you have a mature team rather than foisted upon them.

Developers should commit early and often into their own branch - it is like a back-up system, which backs up at important points, and tracks what you were thinking about when you made the change.

Only merge code-reviewed branches back into the main branch.

Distributed SCM systems have many benefits, and I don't want to sound like I am against them, but if you aren't using one, that's no excuse not to use branches per developer.

Answer 6

Use branching.

Example:

Devs work on their own branch on a feature branch. Review in the changes when merged against the mainline development branch. Merge in the dev branches against release branch when releases are made.

Answer 7

You should take a look at this Google TechTalks by Linus Torvalds: http://www.youtube.com/watch?v=4XpnKHJAok8

Linus is explaigning why Distributed SCM (like Git or Hg) lower the psychological barrier to commit, thus allow devs to "commit early, commit often" (using branches, thanks to merging being so simple in git).

Then, the peer review is done by others dev pulling in your branch into theirs, checking whatever they want to check, and running tests suites.

Finally, when everyone is happy, you can push to a "live" or "pre-live" branch which should be maintained by a thrusted team/person.

Just say something like "It's not me, it's Linus telling it!", it should help a lot :)

Answer 8

We use the strategy described in Version Control for Multiple Agile Teams - works also for a single team - and it works great for us.

Below, an illustration for a whole iteration:

And a very short explanation:

• The trunk contains DONE DONE stories, it has a releasable policy (can be released at any time).
• Development is done in work branches (one per team), they have a softer policy (unit tested).
• when a story is DONE, it is pushed from the work branch to the trunk.
• Merges from trunk to work branches are done every day.

We include "code reviewed" in our "Definition of Done" so a story can't be published from a work branch to the trunk if the code hasn't been reviewed. However, people CAN commit early and often in the work branch of their team (as long as the code is unit tested).

I've used this approach on several projects, multiple size (small project with a single team to huge project with multiple teams), with different VCS including Subversion with success. And I recommend reading the whole paper.

Answer 9

Can't comment so - the strategy with different levels of branches (Pascal's answer) is great. However, I think one thing you would want to clear up is whether "commit" equals "release". It shouldn't.

What we have been doing with a single team per project was that developers commit when ready, however the commit must not break unit tests harness (similar to "unit tested" policy in the workflow from Pascal's excellent answer). We have a test server running straight from the repository and updating after each commit (we do web apps - this is equal to having a build of your software every commit or every day). Everyone can access that server, including the POs and stakeholders - in fact anyone who wants can play with it and provide feedback.

Now, code reviews are assigned as explicit work on the sprint backlog to each item. Items that have not been through code reviews are not considered completed and are not part of the shippable increment that is released at the sprint's end.

In this scenario "committing" means sending your code to the repo. Your code will be visible to others, it will be also visible working on the test server (if it won't break something). The psychological barrier is much lower than if committing meant sending something that is considered releasable. Still, each developer knows their code won't be really released before a code review and they will have to wear a funny-looking hat for the rest of the day if they send a commit that will break unit tests.

As for code reviews taking time - well, this is time well spent, time invested towards higher quality of the product. Well worth it.

Answer 10

If you're looking for a blend, may I suggest adopting a new policy: that code produced in pairs doesn't require a code review.

Pair programming provides the same value as a code review, allows excellent knowledge transfer, and helps novice programmers (and sometimes experienced programmers!) learn quickly. Of course, it brings its own challenges... none of them insurmountable.

Answer 11

The team I am on has adopted a similar policy that no code is committed without a review, but the way we address that is to do almost exclusive pair programming. Some might argue that that does not constitute a "review", but it does ensure that at least two developers went over the code, design, test. On check-in, both reviewers enter their initials for tracability.

Not sure if that will work with your review board, but something you might want to consider.