iHacky Code Review

General Thoughts

Overall, the system is in a very rough state. Functionality of the system is very limited and very basic. It appears that most of the work has been done on the back end setting up the database, server, and the integration with Facebook. Now that the back end is up and running, it is now time to work on the interface itself.

The Code

I have don't have much experience with PHP or with FBML so I cannot really take apart the code. One issue that I did notice was that some of the PHP pages have little or no commments. Internal comments are very important to new developers so each page should contain the necessary comments to understand the code.

System Issues

Understandably the system is in a rough state at the moment, but the aesthetic appearance of the system is severely lacking. At this point in time, functionality is far more important than beauty, but at some point the aesthetic appearance of the system should be addressed.

Also, perhaps one of the most important links on the system, the Need Help? link, does not work. Another issue with the system is that it is not possible to change your Hackystat account once you have set it.

Documentation Issues

The documentation looks pretty good overall. I didn't try to install the system on my own computer, but the documentation to do so looks pretty decent. One problem that I saw with the documentation was that the screen shot on the iHacky homepage doesn't look at all like the current system. The screen shot should be changed to reflect the current state/look of the system.

Also, in the InstallationGuide it does not specify that you have to be logged into Facebook in order to add iHacky. I didn't bother registering with Facebook because I already had a Facebook account. Since the guide did not specify that you had to be logged in I just clicked on the link. When I did so I got an error page saying that "the page you requested was not found". Normally, Facebook says the page you have requested requires that you log in if the page exists. I'm not sure if that is a problem with the way Facebook handles applications or if it is a problem with iHacky's application page, however, if I was just a regular user who has no idea that the application actually exists, I would have thought that the application was broken and would not have bothered pursuing it further.

Also, I'm not sure if the use cases are still relevant given the current state of the system. If they are no longer relevant, they should be removed or edited to reflect the current system.

Potential Problems With iHacky and Suggestions

There does not seem to be a clear, precise idea of where this system is going based upon the presentation (although it does lay out a general road map), the documentation, and looking at the actual system itself. The use cases provide some ideas of how the system will be used, but I'm not sure if they are still accurate or relevant based on the current state of the system. Hopefully the problem is simply that the use cases and other documentation that lays out the "vision" for the system are simply missing or deprecated. If that is not the case, then immediate action is needed so that the details of how the "specific communication mechanism between each user" will work (for example) can be laid out (along with all the other items mentioned in the road map).

Under the current configuration, the networking feature of the system is not really present. The road map provided in the presentation mentions some ideas on how the networking can occur, but once again it is very general. From the current system, I'm guessing that tools, teams, and projects will be used as a networking area. But even in these areas, the "vision" seems somewhat fuzzy. Like right now, the projects tab takes project data from Hackystat and displays it. But how does this network software engineers together? Are you going to generate links within the Hackystat data or is it just going to be list of projects you're working on while using Hackystat. What happens if the project is not using Hackystat? Will there be a way you can add those projects?

One solution to clear things up might be to hold a brainstorming session to figure out what aspects/characteristics/interests of a software engineer would other software engineers be interested in. This common ground could then serve as the basis for how to network software engineers together. Here is a list right off the top of my head:

• Languages (Java, C, C++, etc.)
• Tools (Eclipse, Visual Studio, Ant, etc.)
• Areas of Interest (GUI, Web Design, Game Programming, etc.)
• Projects (Current, Future, Dream, etc.)
• Growth (What new areas do you want to experience?, What languages do you want to learn?, What tools would you like to experience?, etc.)
• OS Platform(s)
• Experience

Once you have a list then you can build your system around them and focus on providing networking options for all of them.

Conclusion

Overall, the potential of this project is really high. One problem with lots of potential is that it is difficult to narrow the focus. In reality, the narrower the focus, the better, as it is better to do a few things well then to do lots of things ok.

Hudson

The CI Breakdown

To be quite honest I did not know very much about Continuous Integration (CI) until I went through the tutorial and read the article. One reason for my lack of knowledge about CI is that I have never worked on a large scale project that would require CI in order to keep the project manageable. CI can keep a project manageable by building, testing, and verifying the project often (every time the code repository changes) in order to spot issues as quickly as possible. Spotting issues right away allows for these issues to be fixed as quickly and as early as possible which makes them far easier and far more manageable to deal with.

One aspect of CI that I found to be particularly interesting is that CI is only as good as your software engineering practices. I guess in a lot of respects, it is no different than an emma or a findbugs. These tools are quite helpful but they are only helpful if you actually run them. The same is true for CI. CI is only helpful if you practice good software engineering practices like those listed below (description here)

• Avoid infrequent check-ins
• Avoid broken builds
• Avoid minimal feedback
• Avoid spam feedback
• Avoid a slow build machine
• Avoid bloated builds

If you avoid these CI anti-patterns as they are called, then CI will prove to be a truly beneficial tool for you. Overall, I think that Hudson and CI will be a vital tool for me in the future, as it can automatically check to make sure the latest commit to the repository builds successfully, and if it doesn't, immediate action can be taken to fix the problem.

Hudson and the Visual Studio Sensor

I was able to use Hudson to build the Hackystat Visual Studio Sensor project that I am currently working on. Using Ant to build C# projects is somewhat tricky and better handled through NAnt, which is not supported by Hudson. Therefore, I had to go for the command line build option that comes with Hudson. Basically what I had to do was turn the command line instance into an instance of the Visual Studio 2008 Command Line. From there I could just run "devenv %PATH%VisualStudioSensor.sln /Build" to build the system. To set up the Visual Studio 2008 Command Prompt, I copied the contents of the vsvars32.bat file (which sets up the Visual Studio 2008 Command Prompt) into a .bat file which I called buildsensor.bat. At the end of the .bat file I put the "devenv %PATH%VisualStudioSensor.sln /Build" line to build the system. Luckily Hudson defines an environment variable %WORKSPACE% which can be used to get to the files checked out of the repository.

Running the Build on the CSDL Server

In order to run the build on the CSDL server, you would have to install Visual Studio 2008 and set up the path to the buildsensor.bat file in Hudson. This seems like somewhat of a pain to do just to build the system so I am currently looking into various alternatives. One alternative is to use csc.exe which comes with the .NET framework. Basically it is a C# compiler which can be used to compile .cs files. I am not sure how how extensive the compiler is or how it compares to the "Build" command in Visual Studio, but it looks promising and would require much less space (Visual Studio requires several gigabytes). The other alternative would be to tackle C# building in Ant. There appears to be an Ant library for .NET building, but I believe it uses csc as well to build the project. Other than how to build/compile the system, the Visual Studio Sensor project can be setup relatively easily:

1. Select the "Build a free-style software project"
2. Setup the Subversion repository by linking the google code repository and providing the proper authentication
3. Select the "Poll SCM" option under "Build Triggers" and schedule it as "* * * * *" (every minute check the repository for changes)
4. Then under "Build" check the "Execute Windows batch command" option and setup the path to the buildsensor.bat file
5. Then setup the "Post-build Actions" with possible email notification
   

Visual Studio Sensor Code Review

Documentation/Guides:

In general people seemed to like the documentation and guides (user and developer) that came with the sensor. There were three areas that needed further clarification or modification.

The first area was that NUnit and two NUnit tests were included with the original distribution, but documentation for its installation and how to run the tests was not created. In actuality, NUnit should have been removed from the distribution before it was released as the tests serve no real purpose and it would simplify working with the source code. A later distribution, released last night, corrected this issue.

The second area that needed clarification was "VisualStudioSensor - For Testing.AddIn" Some of the people working with the source code had errors saying that their project could not find "VisualStudioSensor - For Testing.AddIn" file. Further clarification of this must be included in the Developers Guide to help those working with the source code.

Lastly, it is apparent now that the sensor is only compatible with Visual Studio 2008 that includes the latest .NET Framework 3.5. The handlers and everything else we were using really hadn't changed much from Visual Studio 2005, so I had assumed (which is always dangerous) that the sensor would be compatible as long as the user had the .NET Framework 3.5. Also, apparently not all versions of Visual Studio 2008 come with .NET Framework 3.5 so I modified the guides to indicate that users and developers may have to install this as well.

Bugs/Limitations

Other than the NUnit reference error, the biggest limitation of the system is that only one instance of Visual Studio can use the sensor at a time. Because Visual Studio does not allow more than 1 solution to be opened in an instance at a time, this could present a real problem for developers who want to work with more than 1 solution at a time. At this point in time, I am not sure how to fix this problem but I will definitely look into possible solutions for this as it is a major system limitation.

Also, several people thought the data collected by the sensor was not very useful and was somewhat limited. I am not sure if this is really a Visual Studio sensor issue or an issue with the Hackystat sensor data type conventions. When developing the sensor we tried to follow the Eclipse sensor quite closely so that we could adhere to the sensor data type conventions. All I can say on this at the moment is it is my opinion that the user shouldn't be so interested in the data that is collected by the sensor, but rather in the ways that the data can be used, measured, and displayed. But having said that, this is something we will definitely look into further.

The sensor does not show it is running when a new solution is opened (and the other solution is closed). This type of feedback would be useful to a user who is working with multiple solutions in Visual Studio and will definitely be added to the next release (if possible).

Suggestions:

• Increased feedback on the status bar (perhaps a verbose mode) to give the user a better sense of what is going on.
• Limit the number of characters per line.
• Add DevEvents when the user accesses help.
• Add DevEvents for debug and release.
• Improve user/developer documentation.
  

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.