Code Reviews

I recently attended a code review at Wharton. We’ve put together some code reviews at my job, but I know that Wharton has been doing formal code reviews for years and I wanted to check out their process to compare notes. For this particular code review they were looking at a small ColdFusion application developed using Squidhead.

I was immediately surprised by how simple they kept it: no projector, no whiz-bang demo, just a handful of well-prepared developers with a stack of notes. Only the developer and a manager brought a laptop (and the second laptop may have been for my benefit, since I had not previously seen the code). The developer gave a brief summary of the application (its goals, functionality) and immediately handed things over to the reviewers.

The reviewers had clearly spent some time with the application, reviewing both the code and the interface. (I asked about this after the review, and one reviewer said he had spent 2-3 hours reviewing what was a relatively short and simple application. It is telling, I think, that attendance was voluntary–the reviewers in attendance were keen reviewers.) They got down to the level of detail where they brought up the date format (e.g. why did the developer use dd-mmm-yyyy instead of mm/dd/yyyy?).

The review was relatively structured while remaining casual in tone. They discussed CFCs, followed by other ColdFusion code, followed by user interface concerns and SQL. Each suggestion was backed up with a reason, e.g. avoid using evaluate() because it can lead to performance issues, or use mm/dd/yyyy because ~60% of the intended audience is most familiar with that format. In general, the topics covered fell into these categories: security, speed, standards/best practices, readability/maintainability, and user interface concerns.

It really seemed like a useful process to me. True, it added 12-18 developer hours to the project, but that is a small price to pay to avoid a possible security breach or server crash (not that any such vulnerabilities or flaws were found). The key, it seemed to me, was in the preparedness of the reviewers, who each came with comments in hand. Their preparedness, in turn, was helped by having basic guidelines as to what issues to look for.