How to Review a Module
of the Arsdigita Community System
by Aurelius Prochazka
and Michael Yoon
The ArsDigita Community System has gone through considerable changes
over the course of its existence. Design and coding rules that were
once thought to be set in stone have changed, and modules must be
reviewed on a regular basis to keep quality high, rather than only
when the module is first introduced.
Another reason why the ACS code base is not as consistent as we would
like is that our programming standards have been propagated through a
largely oral tradition. Collecting material from the Developers Guide (which is the closest thing
we have to a standards document but also contains higher-level
information about user interface design and information design) and
other publications (e.g., Ron Henderson's ACS
Development page), we have now started to publish our standards at
/doc/standards (an abstract URL) so that no one will have to
waste any more time wondering, "So what's the official way to do X?"
Read the standards, and, whether or not, they coincide exactly with
your personal style, please adhere to them. If you disagree with a
standard for substantive reasons, then don't silently dissent; rather,
raise the issue so that -- assuming your argument is persuasive -- the
standard will be changed for the better.
Coverage
A full review will cover all of the following:
- /doc/module.html - this file should warn the code reviewer
if there is anything tricky about the system at all
- /doc/sql/module.sql
- /tcl/module-defs.tcl
- user pages in /module/
- site-wide administrator pages in /admin/module/
- module administrator pages in /module/admin/
Goals
The overarching goal is to produce high-quality software. Consistency,
correctness, and efficiency are recognized dimensions of software
quality. We strive to achieve all of these goals by taking the
following concrete steps:
- Standardize:
- Identify bugs (the common errors list
is a good resource for accomplishing this goal)
- Identify potential improvements (optimization, modularity, user
interface, etc.)
- TEST, TEST, TEST - reading code and documentation alone is
not enough to do an effective code review; you need to see the module
in action, so perform the appropriate Acceptance Test and enhance it if it does
not achieve full coverage of the module's feature set. In the short
term, we will need to be exploring more sophisticated testing methods
(automated testing, load testing).
Output
There should be a concrete artifact from every module review, i.e., a
list of actionable items for the module owner to complete. Once this
list has been provided to him/her, the module owner should publish a
plan for completing the list by a set deadline (this plan need not be
complicated; if the list is short and simple, an email saying "I'll
get these done today" is probably sufficient).
As for what form the list of review items should take, there is no
standard yet. In the past, email has been the de facto
standard, but email is bad for a lot of reasons. The ACS Ticket
Tracker is a natural candidate for fulfilling this function. For
now, use your best judgment; just be sure (one way or another) to get
it in writing.
aure@arsdigita.com