Using commit monitors as a form of code review

Question

I'm working in a small company - four developers, working on a variety of projects. We've been looking at what we can do as cost-effective methods of process improvement, and an idea came up.

Given what we do, we often have single developers working on parts of a system, independently of the other developers. This can have a number of negative affects:

1. A developer might not be fully aware of the context in which a change is being implemented, and make the change in a way that will meet the current customer's needs, but will break functionality that other customers depend on.
2. A developer might make a change that breaks the current architectural design, introducing a dependency that will cause problems in future development.
   1. Other developers might not be aware of how the system has changed, in areas that they have not worked on.

We've talked about doing code reviews, as a way of dealing with these issues. But we've not had much success when we tried. It takes a lot of time to prepare a change for a code review, and it takes everybody out of production while the review is being performed. And the benefits of any review we've tried has been minimal.

We're using Subversion (with TortioseSVN) as our VCS. I've been looking at the SubVersion CommitMonitor tool, and wondering whether it might work as a sort of poor-man's code review. It lists every commit made on the repository, allowing someone to see the changes that have been made, the log messages made for that change, the files that were included in the change, and the specific lines in each file that were changed.

Rather than scheduling a meeting, trying to get everybody together to review every change, we could just have every developer review every other developer's commits, at whatever time was convenient. This would keep every developer abreast of what changes were being made elsewhere in the system, and would have every change reviewed for customer conflicts and design consistency, at a fairly low cost.

If someone saw a problem with the code that was being checked in, he could discuss it with the developer who did the commit, or more likely, schedule a meeting to discuss how the new feature could be implemented in a way that would not impact other users or screw up the architecture.

Anyone else doing anything like this, using commit monitors for such a purpose?

Answer 1

No. But I can give you some tips on making code reviews easier.

1. Make it small. Have a house rule to make sure that code is checked in using small, bite-sized chunks that can be quickly and easily reviewed. Looking at a request to review a 20+ file changeset of 1000+ lines makes my eyes bleed.
2. Make it brief. No review should take more than 1 hour. Focus becomes blurry, and other developers resent having to schedule a multi-hour meeting to review code.

Keep large-scale design reviews out of the picture. A code review is not the time to say, "Oh, dude, you should be using API XYZ and design pattern AwesomeFactory."

I've had amazing experience with Atlassian's tool for online code reviews - Crucible. Sadly, they do not offer a $10 "small team" version of this tool. But you should still look at it for their features - offline CRs with discussion threads on lines of code (and other goodies).

Answer 2

Code review doesn't have to be done by all developers for every change in order to be successful. In fact that doesn't scale so well at all. Especially if every code review is a full meeting.

There are code review tools that can be used. We use ReviewBoard, which is open source, but there are other good ones, both free and commercial.

It sounds like the model you are looking for is a post-commit review, which is a review of code after it has been committed. You can ask for a review from one specific person, or from the whole team, as necessary.

In our team, we prefer pre-commit reviews, except in case of emergency commits. In the commit message in SVN we add a link at the end ("Reviewed by [reviewer] ([link-to-reviewboard])"). That allows us to connect the reviews to the commits.

Answer 3

It takes a lot of time to prepare a change for a code review

Why? A change that takes preparation for a code review is not ready for check-in.

You should only need one developer to perform the review. You'd only need the entire team meeting when conflicting views arise so the group can unify around a common view.