Entry-Level Code Review Procedures?
2008-12-18 – 08:20Since September, half a dozen students at four universities have been rebuilding DrProject (our lightweight classroom-friendly replacement for Trac) on top of Django (a Rails-like web programming framework written in Python). What’s made this project different—and IMHO better—is the use of code reviews. Blake Winton, a local Python hacker, reviewed every single commit that came into the SVN repository in the first couple of weeks of the project. Thanks to his example, the students started reviewing each other’s work as well (Jeff Balogh, a two-time Google Summer of Code veteran who’s starting a full-time job with Mozilla in January, being the most prominent culprit).
It made a huge difference to productivity and code quality, and we’d like to do it again next term, but are wondering how best to implement it. We managed reviews this term by having each commit diff echoed to a mailing list; a self-appointed reviewer would reply to the email with comments, the author of the diff would reply, others would chip in, etc. I thought it worked pretty well (especially relative to the near-zero setup cost), but some students said in the post mortem that some commits got lost in the cracks, while others said they found it hard to track what was going on, since code review threads often turned into design discussions without a signal going to the larger group.
So, my question is, what could/should we do next term without either a big investment in infrastructure, or weeks of retraining? (I have no objection in principle to doing either, but since students only work 10 hours a week on this project, and usually have four other courses on the go, I have to focus on the absolutely smallest thing that could possibly work.) One suggestion has been to prepare a diff and send it to the reviewers’ mailing list before committing, so that reviews happen before code goes into the repo. Another is to pseudo-randomly assign commits to other team members for review (so that nothing gets dropped on the floor), and to use a “three strikes” rule to promote discussion from the review list to the dev list. What would you suggest? What do you think would work for a larger group (say, a class of 50 students, working in teams of 5, each team doing an 8-hour-a-week term-long project in parallel)?
2 Responses to “Entry-Level Code Review Procedures?”
Hi Greg,
[Preamble: Greg asked me to comment on this problem. My bias is that I'm the founder of Smart Bear where we make Code Collaborator, a peer code review tool.]
I don’t think you want to move to pre-commit review because it will block students from working, especially if there are arbitrary delays in the review process. Most of our customers use pre-commit review, but that’s because they have full-time people doing the reviews reasonably quickly.
It sounds like the only real problem is that some reviews don’t get done. This problem is typical of email-based review because of course emails get delayed, lost, and ignored. Any time the review isn’t assigned to a single person, you have the effect where everyone assumes everyone else is looking at the code and it doesn’t get done.
Having the system email the review to a particular person is a good step; if nothing else you should do this. It can be legal to pass the review to someone else by forwarding, but keep all the original so there’s a record. You need a record not so much for audit purposes as to know when this review started and how many times it got delayed.
Still, it’s easy for reviews to just not happen.
Of course this is the point where I start pitching a tool.
Seriously though, that’s part of the point of a tool — to not drop reviews on the floor. Reviews stick around until they are done.
Since you’re using Python anyway, consider using Review Board. It’s open-source, Python, and supports the type of workflow you’re describing. Since it’s open-source you can make changes to it should it be necessary. Installation is non-trivial, but you only have to do it once. Training is minimal — it’s not difficult to use.
Of course our own Code Collaborator would work great too, but Review Board will do the trick.
Hope this helps!
By Jason Cohen on Dec 18, 2008
Another vote here for Review Board. We switched to it at work about a year ago and it is a day and night improvement over email threads. Review Board actually makes reviewing code a pleasant experience. It lets reviewers easily tag particular lines of code with comments, and an email thread is generated for each review.
My feeling is that the easier it is to review code, the more interest you’ll see from students. IMO post-submit reviews don’t make sense—reviewers are less interested in critiquing code after the fact, and there is less motivation for the submitter to submit follow-on changes to incorporate feedback.
If you review code before it’s submitted, there is opportunity for some back-and-forth. The submitter can take into account feedback, improve the submission, and re-post new diffs. Lots of obvious mistakes and pitfalls can be caught this way.
Here is a rough description of the process I have used in the past: (1) Dev mailing list is CC’d on the review. (2) 1-2 specific people are on the To: line. (3) Any team member can review the code. The people in the To: line are suggested reviewers. (4) The submitter generally waits to get at least one “Ship it!” before submitting.
If no one is reviewing, the submitter can send a ping message and say something like “I plan to check this in by 5pm today unless there are any objections.” If you really don’t have a lot of interest in reviews, you can make a policy that any review more than X hours old is fair game to check in.
A side benefit of Review Board is that it acts as a place to backup work. If you post a diff on Review Board, it is as safe as checking in—you can always re-download the diff if you lose your work.
By Alan on Dec 20, 2008