Monday, November 9, 2009

Code Review for WattDepot Cli - Part 1

This week, I was assigned to review two other team(Ekolu, Umikumaha)s' source code for WattDepot Cli project. To conduct this review, each of us followed a review checklist that was provided by my teacher. After reading through other's code, I finaly understand how important the Javadoc is. Javadoc will show the structure for the project and it provides a easy way to access the code. Although it is not a fun thing to review code, I had some good time when I tried to break other teams' program.
Anyhow, following is my review for team Ekolu:
A. Verify passed. Build successfully.
B. All commands are working correctly, but there are a number of problems.
1."list sensordata SIM_KAHE_1 day 2009-11-10T23:30:00.0000" works the same as "list sensordata SIM_KAHE_1 day 2009-11-10".
2."list power generated SIM_KAHE day 2009-11-01 sampling-interval 0 statistic max will break the system and no command can be processed after that.
3."list power generateds SIM_KAHE day 2009-11-01 sampling-interval 60 statistic max" works fine. There is no check for string like genterateds.
C. The Javadoc for this project is pretty good. It does not explain how packages and classes relate to each other, but it is still very helpful.
D. Some class names are not very clear. For example, PowerTimestamp is not as good as CommandListPowerTimestamp. Some internal names are not informative too. For example, "fos" is not a good name to represent FileOutputStream.
E. The testing coverage is ok. However, there are some cases do not have test.
1. No test for correct intput for every command.
2. No test for help.
3. No test for sampling-interval value.
4. No test for "list power consumed", "list total energy" and "chart power consumed" command
5. No test for "statistic min" and "statistic average"
F. Overall the package level design is good.
G. The class level design has some problem.
1. SourceListing class is not used.
2. The block of condition: "" equals to sourceName in "list summary" command is a dead block.
H Most methods in the system accomplish only a single task. Some methods need refactor. For example in SourcesInformation class, the if statement in listsources method should be refactored. Overall, most methods do not have side-effects.
i.The consistency is not very good. For example, the error message for "list power consumed SIM_KAH timestamp 2009-11-01T00:00:00.0000" should be the same as "list summary SIM_KAH". Also sometimes stringbuffer is used to concatenate strings and sometimes just use + to concatenate strings.

The review for Umikumaha is still on-going.

I hope my comments can help them to improve the quality of the project.

No comments:

Post a Comment