Sunday, November 8, 2009

Code review of wattdepot-cli-umikumakulo

This is the wattdepot client interface of team umikumakulo (or thirteen) by George Lee and Yichi Xu. Below are some issues that I identified.

A. Review the build: the system successfully build.
1. Missing some external libraries in Eclipse due to the incorrect build directory.

B. Review system usage: there is no problem with the happy testing for all the commands.
2. The error message is invalid. The below is error message should be "Invalid source" instead of "Connection error".

>list summary SIM_KAEH
Connection error

3. Also, the system does not validate the input string properly as input string such as the following are also work.
list sourcres!@#@
list summary23
list power232s ... day
4. The empty command issue: the system should let go of the empty command instead of display "Unknown command" message.

C. Review the JavaDocs: there is no problem generating the Javadocs for the system.
5. The description of the main command class is meaningless. For example, chartPowerCommand: Implements the chart command. Instead, the description of this class should be "Creates HTML file representing chart for power generated or consumed from startday to endday.

D. Review the names: there is no significant issuer with the naming convention in the system.

E. Review the testing: the result of code coverage is over 90% for method, block, and line.
6. Mostly the exception block is not covered.

F. Review the package design:
7. The help and quit command should be part of the command package instead of processor package.

G. Review the class design:
8. The commandProcessor is not self-contained because it also trigger the help command and quit command differently from the other commands.

H. Review the method design: after reviewed the source code of the commandProcessor class, I found myself the answer to the issues the I mentioned in part B.
9. Instead, the commandProcessor should check for each term of the input token (maybe just ignore the case).

I. Check for common look and feel:
10. From the Javadocs, George and Yichi refers to the system sometime as "command line interface" or "command handler". Though, it's not so important for the functionality of the system, it still represents some confusion to the user.

11. Also, after reviewing the source code, there exist some uncommonality in the format. Some blocks of the source code space out widely, which make it more clear and easy to read, while some blocks does not have any whitespace at all.

I hope my comment will help George and Yichi to improve the wattdepot-cli-umikumakulo.

No comments:

Post a Comment