Sunday, November 8, 2009

Reviewing Code Part 2

In this second part, I'll be reviewing code written by Aaron Herres and Dean Kim.

A. Review the build.
Downloaded and ran 'ant -f verify.build.xml'. Seems to run fine.
B. Review system usage
To start the system, it requires the WattDepot URL. The current service could be used as a default.
Got a stack dump when getting the summary for SIM_WAIAU.
>list source SIM_WAIAU summary
Exception in thread "main" java.util.NoSuchElementException
at java.util.AbstractList$Itr.next(AbstractList.java:350)
at java.util.Collections.min(Collections.java:570)
at org.wattdepot.cli.ListSourceCommandCli.getSourceEarliestDataTimestamp
(ListSourceCommandCli.java:374)
at org.wattdepot.cli.ListSourceCommandCli.processSource
(ListSourceCommandCli.java:453)
at org.wattdepot.cli.ListSourceCommandCli.processCommand
(ListSourceCommandCli.java:487)
at org.wattdepot.cli.ListCommandCli.processCommand(ListCommandCli.java:76)
at org.wattdepot.cli.CommandLineInterface.processMainCommand
(CommandLineInterface.java:209)
at org.wattdepot.cli.CommandLineInterface.processUserInput
(CommandLineInterface.java:172)
at org.wattdepot.cli.CommandLineInterface.main
(CommandLineInterface.java:269)
I also tried the following list commands, but it said they were invalid.
>list sensordata SIM_WAIAU timestamp 2009-11-01
The input string was invalid.
>list sensordata SIM_WAIAU day 2009-11-01
The input string was invalid.
I tried the chart command, but got the following results:
>chart power
chart power error. Usage: chartpower [generated|consumed] {source} {startday} {endday} sampling-interval {minutes}
>chart power generated SIM_KAHE 2009-11-01 2009-11-02 sampling-interval 120 file test.html
No power generated values returned for source
Note that the usage string says "chartpower". SIM_KAHE might not generate power, so I tried to check it:
>list powerGenerated SIM_KAHE day 2009-11-01 sampling-interval 120 statistic max
Exception in thread "main" java.lang.NullPointerException
at org.wattdepot.cli.CommandLineInterface.processUserInput
(CommandLineInterface.java:173)
at org.wattdepot.cli.CommandLineInterface.main(CommandLineInterface.java:269)
Finally, I used the list carbon|energy command:
>list total energy SIM_KAHE day 2009-11-01 sampling-interval 120
list total error. Usage: list total [carbon|energy] generated {source} day {day} sampling-interval {minutes}
>list total energy generated SIM_KAHE day 2009-11-01 sampling-interval 120
1.5854300700617285E9
The first command is the syntax listed in the help command, so that should be changed.
C. Review the JavaDocs
The system summary looks good. The package summary could be improved since the system has changed from the initial simple example.
Interesting that you are not required to put Javadocs for some defined constants (namely in ListSourceCommandCli). I have no problem with it as long as the name of the constant is descriptive enough (SOURCES_COMMAND is not very descriptive).
Method summaries are good enough as far as I can tell.
D. Review the names
I have a minor issue with Cli (or CLi, which is probably a typo) being appended to the end of class names. Seems unnecessary (ListSourceCommandCommandLineInterface?).
ListSensorData#toCLIOutput should be named "toCliOutput" (only the first letter in an acronym should be capitalized).
E. Review the tests
Some classes are not tested at all (ChartPowerCommandCli, ListTotalCommandCli, ListPower, and ListSensorData).
ListSourceCommand seems to be only partially tested. It doesn't seem to get sources with subsources.
F. Package design
All of the files are in one package. Seems like there should be at least a separate package for the command implementations.
G. Review the class design
Some of the command implementations include a fair amount of duplication. For example, ChartPowerCommandCli and ListTotalCommandCli have similar verify and timestamp generation commands. These could be moved to a helper class that is used by the command classes to verify and parse input.
H. Review the method design
Some of the methods in the command implementations are listed as protected when they could be made private.
The system handles exceptions by printing stack traces. It should handle the exception and present a clean message to the user about what happened instead of a generic error.
I. Check for common look and feel
It is somewhat noticeable that two people worked on this. Some command implementations have protected methods while others have private methods that are hidden from the user.

No comments:

Post a Comment