Wednesday, November 4, 2009

Reviewing Code Part 1

Our latest thrilling assignment in software engineering is reviewing our classmate's implementations of the Watt Depot Command Line Interface. One of my instructors once told me a long time ago that it's surprisingly easy to catch someone cheating even if they change their name and a few variables. Everyone has their own style of coding. I'm looking forward to seeing how others wrote their programs. This blog will review an implementation written by BJ Peter DeLaCruz, Wahib Hanani, and Lyneth Peou. A later blog will be written for the rest of the members of the class.
I'll be following this review checklist kindly provided to us by Philip Johnson.
A. Review the build.
Downloaded and ran 'ant -f'. Seems to run fine.
B. Review system usage
I noticed a minor issue with the following command:
> list summary foo
There is no data for foo.
Error encountered when processing source name.
Seems that there are two error messages. Foo is an invalid source, so it should be an error instead of reporting that there is no data.
I also tried the following command:
> chart power generated foo 2009-11-01 2009-11-01 sampling-interval 120 file test.html
Data was successfully written to test.html.
Looks okay, but here's the output:
There probably should be a check to see if the day is equal. It also does not seem to be able to handle my bad source name.
C. Review the JavaDocs
The system summary and the package summaries look good to me.
Note that all command classes inherit the main method of CommandLineInterface according to the Javadoc.
In AbstractTestCommandLineInterface, the Javadoc seems to imply that the class contains test cases when it does not.
I noticed that TestSourceSummaryInformation checks that the number of subsources for "SIM_OAHU_GRID" is four. Because the WattDepot service is still in development, things might change on the server. If a subsource is added to SIM_OAHU_GRID on the server (i.e. SIM_OAHU_WIND), this test might fail.
D. Review the names
AbstractCommandLineInterface is not an abstract class, but instead is an interface. Perhaps it would be better named as "CliCommandInterface".
The parser method of CommandProcessor should be named "parse", as parser is a noun (EJS 23).
CommandLineInterface defines some constants like "SOURCE_MESSAGE" and "STRING_MESSAGE" for error messages. They could be more descriptive to say that they are errors (i.e. "SOURCE_ERROR" or "CONVERSION_ERROR").
The names of the command classes could be changed to better match the commands. The commands tend to start with "list" or "chart", so they could be named to match up better with the actual command (i.e. ListPowerDayStatistic or ListPowerTimestamp).
E. Review the tests
For this section, I ran their, which uses the Emma build tool to check test coverage.
Getting the chart for power consumed does not seem to get tested.
SourceInformation, SourceListing, and SourcePowerGenerated are not covered at all. They don't seem to have any tests (questionable if Help needs a test).
SourceSummaryInformation for sources with properties does not seem to get tested.
Many tests test for bad input, but they don't seem to cover "good" input.
Also, the tests do print out a lot of information by default. Perhaps they could be toggled with a system property.
F. Package design
AbstractCommandLineInterface is in a different package from the classes that implement it.
G. Review the class design
In the CommandLineInterface, there are several constants defined, but they are not used by the CommandLineInterface. Perhaps these constants and the AbstractCommandLineInterface could be combined into an abstract class.
The main method in CommandLineInterface does a lot of input handling that could be handled in CommandProcessor. Most of it probably shouldn't be in the main method.
Be wary of public instance variables. isDebugging probably should not be changed by other classes. A "debug" instance variable and a isDebugging() getter would be better. Not sure what isError is used for, but a similar approach can be used (perhaps with "setError(boolean error)" if needed).
The command classes have methods other than doCommand() that are also publicly accessible and they tend to have similar arguments. Should a user be able to invoke some of the commands independent of doCommand()? Those methods should probably be made private.
Note that the Help class overrides CARRIAGE_RETURN when it doesn't need to.
H. Review the method design
In CommandLineInterface#main, the last continue statement is not needed at all. Again, most of the conditions should be handled in CommandProcessor.
Chart#chartPowerToHTML might be better implemented as two methods, one that creates the Google Chart and another that writes the file.
I. Check for common look and feel
Seems like the code was pretty uniform. I couldn't really see any discernible differences between the implementations.
Phew, I think I'm done now. Check back later for part 2!

No comments:

Post a Comment