Ambient Hackystat Code Review

The code that was reviewed can be found here:
Ambient Hackystat Code Reviewed

Installation and Setup:
Based on the documentation provided in the Developers Guide I was able to successfully install and setup the server for use on my computer. One problem with the code is that a person cannot verify whether the system is actually running as expected since it does not pass the ANT task verify.build.xml. It fails at the JUnit task which is troublesome even though the team had mentioned this during their presentation. The main reason is because it is not documented in their Developers Guide. In fact it says to "invoke ant -q -f verify.build.xml to make sure everything is running fine." If an outside developer were to come and try to use the system, they would think that they have not installed the system correctly. Another problem with the setup and installation is that the team assumes that the user is familiar with Hackystat. While this is not an unreasonable assumption, it may cause problems for a user who has not used Hackystat before as the Developers Guide only points the user to the Hackystat homepage. One idea might be to add more specific information involving Hackystat such as registering and setting up both ant and Eclipse sensors. Since this is an open source project, you would like to try to include as many users/developers as possible by making setup and installation as easy as possible.

Part of the setup task involved being able to change the color of the Ambient Orb from whatever the current color of the orb was to the color GOLD. For good measure, I also changed the orb's animation to HEARTBEAT. The XML file that I used to make these changes is shown below:

<?xml version="1.0" encoding="UTF-8"?> <configuration> <triggerActionPair> <interval>60000</interval> <trigger> <project> <host>http://dasha.ics.hawaii.edu:9876/sensorbase</host> <projectName>Default</projectName> <username>default@hackystat.org</username> <pwd>thisisnotarealpassword</pwd> </project> <triggerType>LAST_BUILD</triggerType> <comparation>EQUAL</comparation> <value>FAILURE</value> </trigger> <action> <deviceType>AMBIENTORB</deviceType> <deviceId>123-456-789</deviceId> <actionType>CHANGE_COLOR</actionType> <value>GOLD</value> </action> <action> <deviceType>AMBIENTORB</deviceType> <deviceId>123-456-789</deviceId> <actionType>CHANGE_ANIMATION</actionType> <value>HEARTBEAT</value> </action> </triggerActionPair> <triggerActionPair> <interval>60000</interval> <trigger> <project> <host>http://dasha.ics.hawaii.edu:9876/sensorbase</host> <projectName>Default</projectName> <username>default@hackystat.org</username> <pwd>thisisnotarealpassword</pwd> </project> <triggerType>LAST_BUILD</triggerType> <comparation>EQUAL</comparation> <value>SUCCESS</value> </trigger> <action> <deviceType>AMBIENTORB</deviceType> <deviceId>123-456-789</deviceId> <actionType>CHANGE_COLOR</actionType> <value>GOLD</value> </action> <action> <deviceType>AMBIENTORB</deviceType> <deviceId>123-456-789</deviceId> <actionType>CHANGE_ANIMATION</actionType> <value>HEARTBEAT</value> </action> </triggerActionPair> </configuration>

No changes to the code were necessary to make the changes occur, but I did add print statements to it so that more information would appear while the server was running.

Overall the system seems very modularized, logical, and well put together. As with any review, however, the issues are of the biggest concern.

Top 5 System Issues:

1. Commenting/User Feedback
2. Documentation
3. JUnit tests
4. Error detection
5. Orb color change time interval

The biggest issue with the system is the lack of internal comments within the code and the lack of user feedback when the server is running. For someone who was not involved in this project, it was difficult at times to go through the code to figure out what exactly each part did as there were no internal comments to provide the necessary and helpful descriptions. The Javadoc comments were pretty good, but I was often forced to literally figure out what each part of each method did if the method was lengthy or if the purpose of certain parts were not clear from the Javadoc comments.

When the server is actually running, it would be nice to see more helpful user feedback (or at least provide an argument to activate user feedback) so that users can see what is going on as the system is running. Right now, once the server starts up (I am using the Last Build trigger) all I get is "INFO: LastBuild: active. org.hackystat.ambientdevice.trigger.LastBuildTrigger activate". When I first started using the server, I didn't even know what this meant so I had to go into the code and look to see where these statements came from.

Documentation issues (somewhat understandable at this point in time) needs to be brought up to the level of code. At this point in time, the code seems to be way out in front of what the documentation covers as even the types of triggers that are implemented and are shown in the Triggers wiki page do not match.

JUnit testing remains a large issues as the potential developers who check out the code cannot determine whether the system is functionally correctly or whether their setup/installation is incorrect.

Some error detection needs to be added to the main method to catch errors in the configuration.xml file as it is pretty easy to cause the system to crash right now. All I did was change the trigger type to an invalid enumeration and I got an ugly error message (although a developer should be able to figure out what is wrong) with no recommendation on how to fix it.

The orb takes way too long to change color and/or animation. I've heard from another person testing the system that it took nearly an hour for the orb to change color. On average it seems to take somewhere between 10 and 20 minutes for it to change. Either way, the lag time is a problem that may not be able to be resolved, which raises the issue of how time sensitive triggers, such as commits, will be handled.


Recommendations:

Solidify the interfaces so that they actually help determine what needs to be in each class that implements them. Right now, there seems to be some inconsistency between the different triggers (for example) as far as what is included in them--the CommitTrigger.java is missing the getProject() method--and what needs to be in them.

Remember that this is an open source project so the 3 Prime Directives need to be followed thoroughly. Keeping these prime directives in mind will really help with the documentation and commenting issues that currently exist.

JUnit testing issues must be resolved so that verify will pass. When I was working on it earlier I was getting this JAXB error which I managed to correct by updating Java 6 to update 5. Currently I am getting this org.apache.xerces.dom.DocumentImpl.getXmlStandalone()Z error which occurs when the HackystatTriggerTestHelper.java tries to get a new instance of the sensorbase. So basically, maybe post the problems your having with JUnit to the class discussion board so that we can all take a look at it and see if we can solve it? Because right now I am not even sure I am getting the same errors as you guys are.