Tuesday, November 24, 2009

Code Review of the Ekolugical Carbonometer

Our latest task was to do a review of another group's web application. This blog is a review of the Ekolugical Carbonometer by BJ Peter DeLaCruz, Wahib Hanani, and Lyneth Peou. I purposefully did not include any of the suggestions made in class (use whitespace better, remove names at the top, and adding some information about what the system does). This should be a really short blog, right?

The review checklist I used to evaluate the system can be found here.

Review The Build

The system builds correctly without issues.

Review System Usage
  • The proper format of the date is not listed on the front page (i.e. yyyy-mm-dd).
  • If we put in a bad date, the error message should suffice (we don't need to see a table full of N/A's).
  • Inconsistent output, why is 9:00 red when 23:00 is yellow? The value at 23:00 is higher than the value at 9:00.
  • There appears to be an empty table cell at the bottom of the output.
  • The output has big bold letters compared to the "Enter a date" label and the error label, which looks really small. There should be some balance between the two.
  • I would prefer that there be a label that shows the day we're looking at.
  • Overall, the speed is slow with little feedback.
Javadocs
  • Thresholds.java has a public constructor, but the Javadoc says it's private.
Names
  • Some instance variables are named with all caps when they are not constants. This is especially noticeable in the Session class with the results lists.
  • List variables in the session should be named the other way around (i.e. todaysTimestamps instead of TIMESTAMPS_TODAY).
Testing
  • Coverage is outstanding as far as I can tell.
  • While WattDepotCommand is covered by testing the web application, I would like to see a separate test for it to make sure it works correctly. WattDepotCommand really should be a separate component (as suggested in the next section) and should have its own test.
Package design
  • WattDepotCommand is independent of Wicket and should be in a separate package.
Class design
  • Instance variables for the WattDepotCommand class should not be public. For the most part, you do not want some of these variables to be changed by other classes. Instead, they should be private with an appropriate getter. For example, noData should be private and a method like "hasNoData" should return the value. If you really want someone else to be able to modify noData, then you should have a setter "setNoData(boolean noData)" (EJS 71).
  • Also, why does WattDepotCommand have lists for the results? It doesn't seem to do anything with them.
Method design
  • When the values from WattDepot are added, there are three separate implementations of onComponentTag when a label is created. I suggest creating a class that extends Label and allows you to select a color to put into onComponentTag.
  • I also noticed that onComponentTag does not have a @Override tag associated with it, which it should to ensure you override the method.
  • WattDepotCommand#getCarbonContentData seems to work by appending to a passed in list. Thus, there is an assumption that the results parameter is an empty list. It seems that what you really want is to return a new list of 24 values, or at least one that matches up with the timestamps. The method would make more sense if you returned a new list of results instead of appending it to the results parameter. Alternatively, you can assert that the list is empty and throw an exception if it isn't. The former is the better option though.
  • In Timestamps#createTimestamps(timestamp, tstamp), there's a temp parameter that is set and incremented, but otherwise doesn't seem to be used at all. It should be removed.
Check for common look and feel
  • As in the review of the command line client, the code definitely looks consistent.
Review the documentation
  • Documentation looks good. One thing I would've liked is a link to WattDepot since you're obviously using that service to get your data.
Review Software ICU
  • Overall their stats look fine. The churn level seems to be on the increase, but it looks good otherwise.
Issue management
  • There is a relative lack of issues for this project. If I just go by the issues in the tracker, then only Lyneth appeared to do any coding and Wahib just wrote the user guide. I'm sure that's not actually the case though.
Review CI
  • There does not seem to be any commits from the 19th and the 20th. Also, the 21st has a period of 3 1/2 hours where the build was red. I recommend that you check with Hudson when you commit and make sure the build is not red.
Summary

I think you guys know what you need to do based on the discussion in class, so I won't repeat those. My additional suggestions are to remove unnecessary elements (empty table cells, extra table rows, table full of N/A's, reduce the font, etc) and use issue tracking more. Also, review the design of the code (naming, methods with side effects, etc). Obviously, you guys work well together, even if the Google Code issues doesn't reflect that. Learn to use it so that if someone else comes along, they can jump in and know what's going on.

No comments:

Post a Comment