22.MyISERN-1.1.Review

Project Reviewed:

MyISERN-1-Brown
Author that I am Reviewing: Kevin English

1. Installation Review

I was able to download the system as a distribution .zip file. However, I was not able to download the latest distribution from Kevin's Engineering Log (was the older version), and I had to go to the team's project page to get the latest distribution. The system allowed for the creation of a jar file "myisern.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.jar" and a usage message appears that shows what args are supported. It also prints out a message that says "Thank you for shopping at Walmart".

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

The code is very well organized into distinct modules. Other than the Cli.java class which contains some consistent errors, I see no other significant format or convention errors.

File	Lines	Violation	Comments
MyIsernModel.java	105, 113, 121, *	EJS-9	Use meaningful variable names
MyIsernTable.java	40, 58, 74	EJS-9	Use meaningful variable names
Display.java	30, 46, 60	EJS-9	Use meaningful variable names
Cli.java	30, 46, 60	ICS-SE-Eclipse-2	Left curly brace on preceding line; right curly brace on its own line
Cli.java	52, 60, 190, *	?	White space between methods

3. Test case review

Black Box Perspective

Black box testing looks good. All the classes are tested by generating equivalence classes and checking for the expected output or return values. Most of the testing does not involve testable boundary conditions so the black box testing looks very good.

White Box Perspective

The white box testing is not quite as good as the black box testing. Here are the results from running Emma:

Emma Coverage Summary
class:	92%	11/12
method:	89%	62/70
block:	88%	2020/2293
class:	82%	318/390

As is shown, the coverage across the lines of code is not great and several methods are never tested. Based on the Emma coverage summary and actually looking at the report, the white box testing does not fully cover the code. Most of the problems with the white box testing occurs in the Cli.java class and is due to the fact that not all the possible argument input types are tested. So to improve white box testing, I would simply recommend testing all possible argument input combinations, just to double check that the system is in fact executing the right code based on the input arguments.

Break da buggah

Although the white box testing is not as complete as one would like, because of the way that the command line arguments are handled, I cannot think of a way to break the code short of actually going in and manually rewriting code to produce an error.

4. Summary and Lessons Learned

After looking at Kevins's code I realized that the code I have written thus far for MyISERN is not modularized at all at the class level. My group has written different methods for the various tasks, but everything is still in the class MyIsernXmlLoader.java. After looking at Kevin's code, it dawned on me that only code pertaining to loading MyISERN information from XML files should be contained in the class MyISERNXmlLoader.java. So as soon as we implement the new input ability for the system, I will work on modularizing our group's code into intuitive class modules. Not only does it make the code much more logical in terms of organization, but it also makes modifying it much easier because everything is split into logical parts (and other parts will likely not be affected if one part is changed).

I've also learned that you don't necessarily have to handle command line arguments in the main method of one the classes you implement to perform a task. Instead, you can implement your own class that handles all the command line argument intricacies and then executes the necessary code based on the results of the checks that are performed. This seems like a very logical and modular design that I will definitely consider including something like it in my own group's code.