16.MyIsernReview

Project Reviewed:

MyISERN-1-Orange
Author: Michal Zielinski

1. Installation Review

I was able to download the system as a distribution .zip file. The system allowed for the creation of a jar file "MyISERN-1-Orange.jar" which could be used to execute the system instead of having to use ant or Eclipse. It was not difficult to figure out how to run system, I simply had to type the command "java -jar MyISERN-1-Orange.jar" and the 3 tables appeared.

Here are the results from invoking the following ant tasks:

JUnit ran successfully

Checkstyle ran successfully

PMD ran successfully

FindBugs ran successfully

ant -f verify.build.xml ran successfully.

2. Code format and conventions review

One thing I noticed about the code is that the code used to generate the 3 tables is placed in the main method. Even for a small assignment like this one, I would recommend moving the code to one or more non-static methods and if anything, use a static method to generate the GUI.

Another issue that I noticed is that the code is very case specific. The code used to generate the 3 tables knows exactly how many rows are in each table. This means, that if more organizations, collaborations, or researchers are added, the code will need to be changed for each addition.

Another issue is the commenting of the main method. Two types of comment markers are used: "//" and the "/** * */". Since the comments are within a method, only the "//" type should be used. Also, the comments are very general and do not really add anything to the code.

One additional issue I noticed in the main method is that the toString() method is called on several String objects even though it is unncessary.

3. Test case review

Black Box Perspective

Several MyIsernXmlLoader objects are created and all the methods are tested. Because all the newly implemented code is in the main method, the only way to test it is to invoke the main method. If the code is taken out of the main method and placed in individual methods, the code can be more effectively tested (if they return something). Since the main method is a void method, it is much more difficult to conduct black box testing since the return values cannot be tested.

White Box Perspective

Several additions were made to the main method to allow the organization, collaboration, and researcher lists to be tested to see if they contain the expected values. This is a good test to make sure that the getCollaboration(), getOrganization(), and getResearcher() methods are returning the expected lists. Combine that with the tests that make sure the collaborations, organizations, and researchers sizes are accurate, and everything needed to create the tables has been tested. Since, the main method contains all the code to create the tables, however, whether it actually creates the proper tables cannot be effectively tested. To fix this, I would recommend moving the code to create the tables from the main method to their own non-void methods. This will allow the return values generated from these methods to be tested against their expected values.

Break da buggah

I modified the researchers.example.xml file to add a fourth researcher to it. I modified the TestMyIsernXmlLoader.java by changing the expected number of researchers to 4:

assertEquals("Check researchers size", 4, loader.getNumResearchers());

I then ran the ant -f junit.build.xml task and it passed successfully. When I recreated the jar file and ran the jar, the fourth researcher did not show up in the table. This is the expected result, however, because the size of the researcher table is hard coded into the system instead of being based on the number of researchers in the researcher.example.xml file.

4. Summary and Lessons Learned

After looking at Michal's code I realized just how difficult void methods are to test. A lot of the time, there is no effective way to test the method using JUnit because no values are returned. And if there is a lot of code in the void method, then that can make JUnit testing pretty worthless. In this particular case, testing can be done on the void methods visually by inspecting the tables and visually checking to see that they have the rights results in them. However, this defeats the point of using the JUnit ant task.

Another interesting lesson learned from this exercise is that just because the various ant tasks run successfully on your computer, does not mean that they run successfully on someone else's computer. Michal emailed me saying that the tasks run successfully on his machine and provided a screenshot as evidence. However, when I ran the tasks on my machine, the findbugs.build.xml task had 1 warning. At this point, the only thing I can think of is that maybe we are using different versions of findbugs? Irregardless of the discrepancy though, the warning that it generated was correct since calling the toString() method on a string is unnecessary.
After further review, it turns out that I was using FindBugs 1.2.0 and Michal was using FindBugs 1.2.1. So one lesson learned is that it is important that everyone is using the same version of all software engineering tools

I've also learned from reviewing Michal's code, that the code I came up with for my own system needs to be revised so that it can be better tested. I will revise my code by adding another return method and by making my methods return testable values. This way, I can write better tests for my system which will better insure that it is functioning properly.