Technical/Code Review Checklists

You can use the checklists below for reviewing your own code or someone else's.

In General

• Check for overlap with existing ACS functionality, whether in the data model or in the code. i.e., does the code being reviewed duplicate some existing ACS feature, or does it fail to use ACS functionality when it should?
• Remember the key principles of DRY, Don't Repeat Yourself and Orthogonality. DRY is self explanatory - the more code gets repeated, the more difficult it becomes to maintain and fix. Orthogonality is the idea that one should eliminate effects between unrelated things.
• Take the review seriously, meaning that everyone on the review team should be prepared. Don't be in the situation of reading the code for the first time during the review meeting, which guarantees that the review will be slow, tedious, shallow, and generally inefficient. Bottom-line: Respect the time and efforts of your colleagues.

Data Model Checklist

• The data model is commented at the top with the current maintainer/author's name, email address, and a brief statement of purpose.

• Comments are present and accurate for any part of the data model that is less than self-explanatory.

• The data model is in 3rd Normal Form, i.e. "Every nonkey column provides a fact about the key, the whole key, and nothing but the key." If there are deviations, i.e. parts of the data model are de-normalized, there should be clear comments and justifications for breaking normal form.

• Column names are sensible and conform to ACS standards. In particular, tables that will likely be audited all have the auditing standard columns (creation_date, creation_user, modified_ip_address, etc.)

• Data model constraints are named according to aD standards, and are sensible for the problem being solved.

• The _map suffix is used for many-to-many mapping tables.

• varchar primary keys are avoided wherever possible.

  Optional

• An Entity-Relationship diagram is available to supplement the written data model.

• There is a script available to drop the data model cleanly.

• Appropriate indices have been defined - ensure that potential performance bottlenecks are at least recognized if not addressed.

PL/SQL Checklist

• Since formatting is enforced even less for PL/SQL than for Tcl files, make sure that it is at least consistent and reflects the logic of the code. A style guide is coming shortly.

Code (Tcl/AOLserver) Checklist

• Pages should conform to style guidelines stipulated in Developer's Guide.

• There should be no Common Errors.

• uplevel and upvar should be discouraged - they're seldom necessary.

  Forms

• set_form_variables or set_the_usual_form_variables is done first (or nearly first), and comments indicate what variables are expected to be passed in (as either required or optional). Alternatively, ad_page_variables is used - there is as yet not a strong consensus on this topic, though we can expect to move towards the ad_page_variables approach in the future.

• Input checking is present wherever necessary, for nulls, types, ranges, date validity, etc. ad_page_variables seems to be the wave of the future here.

  Security

• Modules typically need to define a module admin group. Ensure that checking for module administrators also respects site-wide admin privileges, typically by using ad_user_group_authorized_admin_or_site_admin user_id group_id db

• Ensure that if they must be used, the Tcl eval and exec commands have no unchecked user input passed to them.

• Avoid passing anything but primary keys and user input between pages.

• Don't pass any SQL via forms between pages.

  SQL

• select * from... is almost always trouble - the only place it might be useful is for views, and seldom even then. Specify column names explicitly.

• $QQvariables or DoubleApos is used for all strings. In fact, at least with Oracle, everything could be quoted/validated.

• Formatting and style must be consistent - here's a set of guidelines by Justin Ross.

User Interface and Experience

• ACS modules should conform to our style guidelines.

Last Modified: review-guidelines.html,v 1.1 2001/01/21 01:49:08 bquinn Exp