Code Review: Ambient Device Project

The source code and powerpoint for the ambient device project.

Here is my review for the current code base of this project:

I. Passing the verify.build.xml
The verify functionality is provided, however, it fails on the JUnit. Although the team clearly mentioned this both in wiki and the presentation, I was not surprised by it. What really surprised me was its fail on the PMD violation:

C:\ambientHackystat-0.1.303\src\org\hackystat\ambientdevice\device\orb\AmbientOrb.java:20 A class which only has private constructors should be final

However, this is NOT the fault of the ambient device team. The reason for this is because I'm using the PMD version 4.1 instead of the version 4.0. Actually, there are some QA tools that I'm using are on the newer version, but PMD is the only one that gave different result. I wonder if it is a good thing or not to use the newer version, since we had the problem of using newer FindBugs before, and the newer version of Emma is still buggy from my experience.

II. Changing the orb's color
This is actually easier than I expected. Well, it still could be hard for a normal user without any technical knowledge. By changing the user name, password, device ID, and value for the color in the XML file, one can change the color by running the jar file provided with the modified XML file. From my experience, the time it takes to change the color can rage from 4 minute to 18 minute. Honestly, I have yet to see its practical usage if it takes that long to indicate any change. Also, I assume the team would provide a better interface later for changing the XML file, or otherwise they should start thinking to provide one.

III. Improvement & Suggestion
Since I'm still not fully understand the structure of the system, I don't think I can give too much suggestions on improving the design of the system. There are, however, somethings that can be improved easily. First, some comment explaining what the code is doing would be helpful. The whole system almost has no comment besides some mandatory javadoc comment, which isn't helping much. Also, from the schema, it seems like that all the triggers have a comparison attribute, which I don't think it's necessary. For example, a fail unit test can just return a value of false, instead of doing a comparison. (Well, I'm assuming that's what it's doign according to the schema)

IV. Top five biggest problem in the system
Here's what I found that is most problematic, although some of them are due to the device itself, not the programmers.
1. The color changing latency is way too long, which makes the device practically useless. Also, many color differences is hardly noticeable by human eyes.
2. Documentation is lacking. Not to mention the lack of comment inside the code, but also the lack of detail in the installation guide. For the purpose of a code review, the developer guide might be sufficient, but the installation guide should at least let people know what they can do by bringing up the server, or more precisely, what would happen by having the server up. I don't see that from the installation when I was doing the review.
3. There are too many unnecessary libraries included in the system, such as the derby, restlet, gwt.
4. There is no real "mesh-up" with the HackyStat framework for the current system. I can see some of the triggers are pulling data from the HackyStat sensorbase, but it doesn't seem like they are working right now for me.
5. Many JUnit testing cases still fail. But since this is only for the code review purpose, I would expect the team to fix them all later.

V. Summary
Overall, the ambient device team has done a pretty nice job of getting the orb to work. They already have the basic structure of their project setup, and what is left for them to do is to refine and put in more features to the system.