Friday, December 2, 2011

A Technical Review

Collaboration is the name of the game when it comes to Software Engineering and an important part of the collaboration process is performing technical reviews on other developers' code.  Here we shall evaluate a group's Hale Aloha CLI project where they attempted to create an open source Java command line interface that would allow users to query a server for various energy / power related information.  This was to be done using the WattDepot library which simplifies the task of connecting to the server and retrieving the data so that the developer can focus on processing that data.  As a result, we will not focus so much on the code itself, but take a look at how "good" this project is by comparing it to the three prime directives of open source software.

Prime Directive #1: Does the system accomplish a useful task?
The first prime directive is pretty self explanatory, does the system do what it is supposed to do?  Here I tested the various commands that the system was supposed to implement and checked if they worked as I expected them to.  This initial release of the system is expected to implement 6 commands: help (return a list of commands), quit (exit the CLI), current-power (returns the current power usage of given tower/lounge in kW), daily-energy (returns energy used by the given tower/lounge on given date), energy-since (returns energy used by given tower/lounge from given date to now), and rank-towers (rank towers from least to most energy used on given interval of days).  All six of these commands are present in the system and for the most part are functional, but there are also a few problems.

The first major problem is that the commands do not work with all of the advertised source values.  The UserGuide lists lounge and telco sources (i.e. "Lokelani-04-telco") as valid, yet the system rejects them claiming that they are not valid.  It works fine with all of the tower  (i.e. "Lokelani") and lounge (i.e. "Lokelani-A") aggregates though.

The system failing to recognize a telco source as a valid source.

Another problem is that help command breaks if the jar file is moved to a different directory.  The current implementation of the help command references an external text file to retrieve its data with a hard-coded path and if the jar is moved outside of the distribution directory, the help command fails to work.  This is a problem since I think that the executable jar file should be self-contained and fully usable from anywhere in the file system.

The help command failing when the jar file is moved.

Yet another issue is that invoking the daily-energy command with today's date does not work.  It does not work as the command tries to retrieve energy data between the start of given date (12:00 am today) and the start of the next day (12:00 am tomorrow) which is an invalid range since the end of the range is in the future.  While the project specifications does not explicitly state whether today is a valid input for daily-energy or not, I expected the command to retrieve data from the start of today until the time of the latest recorded sensor data instead.

The daily-energy command failing to work when given today's date.

Finally, rank-towers does not work when it is given the same day as the start and end dates.  Instead, it will print out just one line for the Lokelani tower with a value of 0.  This is not good as giving the same date should either be considered a valid output and print results for all four towers or be flagged as invalid and raise an error message to inform the user.  Thus, I believe that the current behavior of the rank-towers command when given the same day as the start and end dates is unacceptable as it does not indicate if that input is valid or not and prints an incomplete output.

The strange output from rank-towers when using the same day as both the start and end dates.

Overall, most of the functionality is there.  Outside of these cases, all of the expected commands are present and work as expected by printing out results when given valid inputs and printing error messages when given invalid inputs.  Consequently, I believe that this system somewhat fulfills the first prime directive as long as the user is careful about which inputs is used and does not move the jar file.

Prime Directive #2:  Can an external user successfully install and use the system?
This directive tests the documentation of the project from the view of an external user.  Unfortunately, the home page does not tell the user much about the system and lacks any sample input and output.  Furthermore, the UserGuide does not tell the user where to download the distribution, how to "install" the system (i.e. extract it from the zip archive), and the command to invoke the executable jar file is wrong (the name of the jar is "hale-aloha-cli-kmj.jar" not "hale-aloha-cli.jar").  On the other hand, the UserGuide does do a fairly good job at explaining which commands and arguments can be used (thought there is that error as mentioned in the the previous prime directive), the distribution is labeled with a version number to distinguish it from past versions, and the distribution does contain an executable jar that does not require any building or compilation.  However, the aforementioned problems are rather major so the system only partially fulfills this prime directive.

Prime Directive #3: Can an external developer successfully understand and enhance the system?
The third and final prime directive once again tests the project's documentation, but from a developer's view.  A major document for this directive is the DevelopersGuide which should detail how an external developer can build the system and how to extend it.  While it does detail how to build the system using Ant, it does not tell the developer how to generate the system's JavaDocs.  It also does not mention that new functionality must be accompanied by new JUnit test cases as it only mentions that additions must pass CheckStyle, PMD, and FindBugs.  In addition, the DevelopersGuide makes no mention any standards that are being followed.  The instructions for extending the system are rather vague too as it does not explain exactly what to modify in the CommandParser class to add a new command and makes does not describe how to add any new commands to the help command.  Finally, the guide mentions that the project is being managed in an issue driven fashion, but does not explain exactly what that means for developers (i.e. make a new issue for every new command / functionality).

Outside of the DevelopersGuide, JavaDocs and source code comments can help external developers understand the system.  Overall, the JavaDocs do a decent job of explaining what the functions do and what the expected arguments are even though there are a few misspellings (i.e. Comman's instead of Command's in the Command interface's printResult method's comment), fields are not commented, and the overview.html does not fit match the project.  Conversely, the source code comments are a little sketchy.  Some source files like EnergySince.java and HaleAlohaCli.java include inline comments to tell readers what the code is doing at a glance while others like CommandParser.java have no such comments, leaving external developers to figure out the code on their own.  Another problem is that most of the classes do not support information hiding.  Ideally, all fields in a class should be private and only be accessible through getter and setter methods, but several classes like EnergySince and HelpCommand declare their fields without the private keyword.  This makes them protected by default and allows any class within the package to manipulate the fields which gives external developers the ability to put objects into possibly invalid states and unknowingly break the system.  All in all, this project does not satisfy the third and final prime directive very well as it fails to explain all of the expectations that are in place when extending the system, does not consistently support information hiding, and leaves a few critical things up to the developer to figure out on their own.

Other Expectations
In addition to fulfilling the three prime directives of open source software, there some additional requirements that the system should meet.  One such requirement is that the system should be well tested so that it is easy to find out if enhancements broke pre-existing pieces of the system.  To be well tested, the developers should have created JUnit test cases to check most of the functionality of the system while executing a majority of the code for good code coverage.  Unfortunately, this system fails to do this as it includes only one real JUnit test case.  While there are other "test cases," they do not use JUnit and have a .txt extension so they are not automatically run by Ant when the system is verified.   Additionally, the one JUnit test case that is present only tests three out of the five methods in the class that it tests.  The result is a barely system that has about 18% of its code tested.  Hence, this system does not meet that requirement as there are virtually no tests to determine if any new additions broke existing components of the system or not.

Another expectation was the use of issue driven project management (IDPM) to evenly distribute the work between group members.  In IDPM, members meet every few days to divide the current tasks into small work units called issues which should be tracked on the Issues page of the project.  In addition, most commits to the systems should have an associated issue in its commit log to help explain why that change was made.  By looking at the Issues page it is quite clear who was responsible for what and it shows that the work load was not exactly even as Micah had 7 issues to work on while Richard and Jesse only had 4 issues.  They did not do a very good job at linking their commits to their issues either as only 19 out of 26 commits (~73%) are linked with an issue.  Some of the changes were minor though and can be excused, but that would still put them below the desired 90% of commits associated with an issue.  Therefore, despite their efforts, this group did not quite make the best use out of IDPM as they worked on this project.

The final expectation is the use of continuous integration.  Continuous integration tools attempt periodically to build and test the system (i.e. after every commit) to ensure that the system stays in an acceptable state.  If it fails to pass these tests, the project members are alerted so that they can find and fix the problem.  This project uses a Jenkins server which can be found here as mentioned in the project's DevelopersGuide.  By looking at the Jenkins server, we can get an idea of how development of the project progressed.  There are a total of 9 failed builds, but 5 of them were a result of the server used by the system being down and was out of the group's control.  The other 4 failed builds were promptly fixed though with an interval of five hours being the longest the system was in a failed state which is not too bad considering that this interval was in the middle of the day when the developers were probably busy with classes.  The rate of commits seems rather good too.  Between 11/10 and 11/15 and between 11/23 and 11/26 there were no commits made which implies that no significant work was completed in these time frames.  This equates to about one week of lost time which is definitely concerning, but the rate otherwise seemed quite consistent with a couple of commits every day or two.  Consequently, it looks like this project made good use out of the available continuous integration tools.

The build history of the project as found on the Jenkins server.

Conclusion
Overall, this project has some problems.  It fails to completely satisfy all three prime directives of open source software due to incomplete documentation and weak testing and it also seems to fall short of some of the other expectations from this project as the weak testing lead to poor test coverage and they were not quite able to use issue driven project management optimally.  Despite these issues, their use of continuous integration was good and the overall system works in most cases.  In conclusion, this project could use a few tweaks, but reviewing it as an outsider has been an interesting and enlightening look into the software development processes of other developers.

0 comments:

Post a Comment