Peter Hosey of Adium on Code Reviews

Posted by Jonathan Allen on Oct 09, 2007

Sections

Process & Practices

Topics

Agile ,

Teamwork

Tags

Coding Standards

There is no question that formal code reviews catch bugs and can delay the inevitable "big ball of mud" that all successful projects seem to turn into. However, arranging a meeting for every check-in quickly becomes untenable on all but the most critical of projects. Peter Hosey talks about his experiences and how he conducts code reviews in Adium.

Thanks for taking the time to speak with us Peter. First, can you give us some background on Adium?

Adium is a free open-source multi-protocol instant-messaging client for Mac OS X. It supports AIM, MSN, Yahoo!, Jabber (including Google Talk IM, LiveJournal, and Gizmo IM), ICQ, Bonjour, MySpace IM, and half a dozen others. It's Mac-only (written in Cocoa) and licensed under the GPL.

We use libpurple, a GPL instant-messaging library by the Pidgin project, for almost all of our protocol support.

What prompted you to start doing code reviews on each check-in?

Two things:

(1) Bugs

Over the preceding year or so, I had randomly found a few bugs in the code that made me shake my head and go “how did *this* happen?!”.

Some of them also made me think “man, I wish we had some tests, since those would have caught this”.

I'm not very active in the project myself, but sometimes another developer will show me a ticket (in which case I sometimes go hunting for the bug myself, out of curiosity) or a weird chunk of the source code. That's when I would notice the bug and go “wait… what?”.

(Obviously, this is a good way to see more bugs than non-bugs, since people aren't as likely to ask their fellow developers about code that's been working perfectly.) Eventually, I'd seen enough bugs this way that I decided to cut them off at the source.

I subscribed to the commits list in order to catch the bugs as they're written, by reading the commits as they happen. When I see a bug, I reply directly to its author, pointing out the nature of the bug and usually suggesting a fix. I've caught a couple of bugs this way.

(2) Google Summer of Code

I'd been meaning to subscribe to the commits list for awhile before I actually did it. What made me get around to it was being a mentor again in the 2007 Google Summer of Code.

Last year, I didn't do nearly enough code review, leaving it until the final evaluation; the result was that my poor student basically ended up with a large stack of code-review comments dropped onto his figurative desk. There were a number of bugs in the student's code that I'd have caught much earlier if I'd been reviewing it (sound familiar?).

So for this year, I resolved to not put it off to the end of the Summer. I subscribed to the commits list at the start of the Summer, so that I could read all of my student's commits as they happened.

This worked exactly as I'd hoped, and being able to do this with everybody else's commits as well is a nice side benefit.

Sounds like it is working our pretty good for you. What source control software are you using?

Subversion.

Pidgin* uses Monotone, and their developers (one of them in

particular) are looking to get us at least over to some sort of DVCS, if not Monotone. I'm intrigued, and I think I'll play with Monotone sometime in the near future. That said, the Adium project currently has not discussed the matter and has no official plans to switch away from SVN.

One thing I'll need to look at is what sort of support MTN has for commits mailing lists.

* Pidgin is sort of our sister project: They develop libpurple, which is a library that implements almost all the protocols that Adium supports, and is the library we use for those protocols. libpurple also serves as the core of Pidgin (of course) and Finch.

How hard was it to subscribe to commits?

Very easy. It's just a Mailman mailing list, which gets emailed by one of the commit hooks (presumably, the post-commit hook).

So the subscription procedure is the same as for any other Mailman

list: Either go through the web interface, or email the subscribe address.

Based on your experiences thus far, do you have any guidance on what kinds of projects this is most and least suitable for?

I recommend subscribing to the commits list for any developer on any open-source project. Actually, it'd probably work for closed-source projects as well; the only difference in that case is that the mailing list should, of course, not be visible to the public.

If you don't have a commits list, but you do have Trac, you can get a commits-only timeline feed. Here's the URL:

TRAC_URL_HERE/timeline?daysback=7&changeset=on&format=rss

Example:

http://trac.adiumx.com/timeline?daysback=7&changeset=on&format=rss

Of course, there's a case to be made for subscribing to the tickets as well:

<TRAC_URL_HERE/timeline?
&daysback=7&ticket=on&ticket_details=on&changeset=on&format=rss>

But on some projects, that may be too high-traffic for all but a few developers to deal with. Mostly, if your project is very popular and has a Trac that users can submit their own tickets to (like Adium has), you're probably better off assigning the ticket management to some non-developers and letting most (not all) of the developers deal only with commits (like Adium does).

Also, here's an idea that I just thought of, have not tried, and haven't seen tried: if you have a frequent patch author whom you're considering making an official developer on your project, you might consider having that person subscribe to the commits list and review commits. If they're good at spotting bugs (and maybe coming up with patches to fix them), this may help you be more confident that they'll be good to have on your team.

If you do that, I'd also suggest considering having the provisional developer contact directly the committer who committed the bug, rather than going through the usual patch process. This should help the commits list serve to help these provisional developers integrate more with your existing team, rather than simply serve as a source of more patch opportunities.

 

• Older >