Wednesday, July 2, 2008

Code Reviews

A little while ago, I made a mention about a project called Rietveld (and this also applies to ReviewBoard). Today, I finally got a chance to dive a bit deeper into how these two tools work and sadly, it is the opposite of what I would like.

The issue with both of these tools for me is that they work on the concept of diffs. A developer writes some code, uploads a diff and then waits around for people to approve it. After it is approved, the developer then checks the code into the repository. This is a good model in open source projects where people are contributing patches and do not have access to check in code themselves. It is more like a patch management system. It is also a good model in larger corporate environments where controlling everything that goes into the code base is done with QA teams who test these patches before they go into the repository.

That is not how the company I work for works. We have a more agile development process where we are all expected to have at least a decent understanding of the entire codebase. Thus, we are checking in code all day long and diff are emailed to all of us. The expectation is that we read each others diffs and will comment on them appropriately. This is more of an after the fact type system. Sure, some bugs can (and do) creep in because they are missed in larger commits where we don't read the entire email, but I'm not worried because QA should catch those before they go to production, when they do testing in our preview environment.

What I would like is a system that has a log of all changes that go through the source repository. When you log into the system, you see a list of all the changes that have not been reviewed yet and a list of comments that have either been made about your code or you made about someone's code. This would be similar to the list of the diff emails sitting in a folder in my email account. I then have the option of just ignoring a commit or clicking through to see more details about the commit. The details page has a list of the files which were changed and a diff of any text based changes. I can click on any line and make a comment. That comment then becomes an issue for the developer who made the commit. The developer gets an email and has to respond. The only person who can close out the issue is the person who created it in the first place. Both people are bugged in various ways (emails and in the webui) until the issue is dealt with. The social politics around all of this is that these issues become highest priority above all else because code is already into the repository and a release can't be made until all of these issues are resolved.

Update: I posted to the ReviewBoard mailing list and got a very helpful response, but still isn't quite what I want to see (the UI isn't there to support it). Check out the comments on this posting. Crucible is very close to what I want, but costs $$$. Others are interested in starting an open source project.


Paul said...

Review board can do some of what you are looking for. I think you are looking for a larger more automated way of tracking check-ins and the comments on those check-ins.

We use review board on both pre- and post- commits to perforce (our change management system).

To make the system work close to what you are looking for, each developer after they check in their code(s)... they would need to go back and submit those changes to review board.

I think it would be cool to have a system that automatically adds commits to a master list that you are expected to review. It just doesn't have the automation that you are looking for.

I'm sure if you were really adventurous... you could take a branch of review board and write some kind of utility to auto-submit changes based on a criteria and add specific people to the reviews.

Gary said...

Check out Crucible from Atlassian ( It's not free, but it monitors your code repo for submissions. You can browse through your source code and start a review per change or file.

Jon Scott Stevens said...

Gary, Crucible looks exactly like what I was looking for! Not that it would stop my company from purchasing it, but too bad it is $$$, I always prefer free developer tools.

Jorge Uriarte said...

Hi there, Jon and others; I've started worrying about this same issues some time ago, and I also think Crucible match my own requirements (except not being neither cheap nor open).

I think subversion's branching abilities (and even better, new mergetracking capabilities since 1.5) make for an excelent basis to work on a codereview system.

Think of it as a hook avoiding commits in the target branch, unless revisions in the source branch have been approved...

After that, let's work on a good UI around it, and that's all (well, not all, but a beginning anyway...)
If you wanna scratch that itch... I'd get some time from my company to work on it :-)