22. MyISERN1.1 Review

For this code review, I once again reviewed the code of the blue group. My rationale for choosing the same group is that I wanted to see how the code has improved over the last week.

1. Installation Review

The system didn't install as straightforward, which I believe was largely due to the incorporation of SWT into the code. The idea of putting a GUI into the program is a great idea, and it might be true that this point may have been early to implement it.

Steps taken to get the code to compile:

- Installation of SWT
- Modification of project to remove org.eclipse.swt and added SWT jar files to classpath

At this point, Eclipse was able to run; however, ant at the command line did not. Attempts were made to change the main class, but it didn't work. At this point I gave up and just figured to do all my testing via the Eclipse IDE.


2. Code Format and Conventions Review

Verify didn't pass. Checkstyle reported a redundant import statement - mainly due to the import statement importing the package the file was stored in. (line 7 of TestMyIsernXmlLoader.java)

Aside from this, verify passes.

No major coding violations were readily noted.


3. Testing

Black box
The following tests were used in our (silver) group's code, and was thus considered here:
- Checking unique IDs amongst all lists
- Checking years
- Checking validity of links
- Listing an object for a valid ID
- Listing an object for an invalid ID
- Listing an object for a null ID
- Listing all of a type of object
- Listing organizations with integer parameter valid
- Listing organizations with integer parameter zero
- Listing organizations with integer parameter negative

Also, checking for a non-null name was specified; however, JAXB does this automatically, so it wasn't tested.

White box
EMMA reports 62% overall coverage. The following were areas that weren't covered:

- Parser class. This command processes command line arguments, it seems. It didn't seem to be used much.
- MyIsern class. This is mainly due to the unimplemented GUI interface.
- MyIsernXmlLoader class. This was covered except for the invalid data cases.

Break da buggah
For this test, I took the malformed data that I used for my testing, where the invalid entries would hopefully be noted by the parser. The XML has invalid entries in two ways:

- There are two Philip Johnsons in the researchers file.
- There is an invalid year of 2050 in one of the collaborations.

Result: The duplicate Philip Johnson was detected, but the invalid organization (with the year 2050) was not.

4. Conclusions

The implementation of additional features sometimes makes development a lot more difficult, as in the case with easter eggs. I recall one of my prior ICS instructors mentioning how development frowns upon easter eggs because they are difficult to test since they are hidden in the functionality of the program. In this case, what seemed to be a useful feature seems to be more of a headache, since it made the building of the system more difficult.

This also is a good example of the difficulty of being IDE-independent. To me, it seemed like a lot of the development was done in Eclipse, and running the Ant build files from the command prompt didn't work as expected. I'm also suspecting this relates to the added library, since the creation of the JAR file did work the last time I reviewed this code.

Also, documenting the SWT_HOME environment variable in the documentation would have been helpful. It is difficult to sometimes do this though, since not everyone has access to a clean installation of an operating system.

Finally, I would like to make a comment of keeping multiple copies of a single file in the src folder - this generally is a bad idea. From my knowledge of CVS systems, Subversion keeps your revisions automatically in the repository - this is the purpose of keeping a repository.

On a side note, I still envy the table printout format :)