29.MyIsern-1.3-review

Reviewing Team Purple.
Team Purple distribution available Here


Installation review:

I began, as any user should, by looking at the installation guide available Here. The guide is short with just a few bulleted points. It was all very simple except for a couple points of confusion:

- "run the junit task..." is a bit confusing. I was not immediately certain that this was the command "ant -f junit.build.xml."

- There is no mention about the URL of the project once it is deployed. I had to go into the tomcat manager to find the exact URL of it. It would be helpful to mention this in the installation guide.

Other then the above confusing points, it was quite simple to install and run the MyIsern-purple system.

I then ran the verify ant task. I was quite impressed with the sheer volume of tests running. It seems they tested the project quite thoroughly. Verify, however, does not pass. Some warnings from Findbugs causes it to fail. The 9 Findbugs warnings range from simple redundant ToString() operations to perhaps more complex problems involving non-serializable objects and HttpSessions.


Code format and conventions review:

I was quite surprised by the number of classes that were present in this code. However, the design seemed quite clean. I could not find many code conventions violations, though I did find some redundancy in certain parts of the code:

- The login() method in the LoginActionBean.java file separately checks if the username, password, or both failed. This can all be done with a single check to greatly reduce the complexity.

- In the CollaborationFormActionBean.java file on line 129 the .toString method is used on a String.

- Perhaps some of the class names are too verbose. For example CollaborationFormActionBean is a bit long for a class name.

Most of the code seemed to follow the guidelines quite well. I could not find many violations.


Test case review:

As I mentioned earlier, there are a rather large number of tests for this project. Running the junit task takes around 30 seconds to complete with 42 total tests running.

Black Box Perspective:
Since black box testing should focus on I/O behavior, it must deal with the UI for this type of project. UI testing, however, can be difficult on this type of client-server application. While there were a large number of tests, particularly dealing with login/logout behavior, some of the more basic tests of the linking system were missing. To greater cover black box testing, it may be useful to create a series of tests (perhaps another test class) that checks all links and navigation through the system. A large part of web applications is navigation through the system, so it is important to test it.
There may also be an issue with concurrency, which is nearly impossible to test for. I would think it would fall into the black box perspective since concurrency issues come about when certain I/O events take place at certain times. There are no tests that deal with concurrency, though I do not think it is even feasible to test concurrency.

White Box Perspective:
The emma results were fairly good for this project, considering the difficulty of testing server-side components. The results from emma were

Emma Coverage summary
class: 88% (29/33)
method: 67% (172/255)
block: 61% (1908/3153)
line: 62% (405.1/656)

For many of the classes, general tests were performed over the entire class (or functionality), yet not many had tests for each individual method. The coarse granularity of the tests focused more on Black box testing. For example, the LoginActionBean class only have tests for the SecurityFilter and various standard login events. Each specific method of the class is not tested, as should be attempted with a white box testing perspective. Many very small tests could be added to test each small piece of code. This could increase the coverage as well.

Break Da Buggah:
I think many people had trouble getting the system complete by this stage, so I was not surprised to find an unfinished section of the project. The "about us" link leads to a non-existent page called reports.jsp. This is simply a finishing problem and not an actual problem with the project.
Another small bug I found involved adding new objects. When a new collaboration is being added, the Years list is not filled with any values, so it is impossible to specify what years the collaboration took place during. This is simply a matter of propagating the list even when a new entry is being created.


User Interface review:

At this point in the project, I try not to focus on aesthetics of the pages at all. The "fluff" can be added later once all of the functionality is available.
The basic navigation of the system is quite nice. It is easy to view each list and navigate to the edit page. I like the feature that automatically propagates the edit form when the user clicks on the edit button for a particular row. It reduces mouse clicks and enables users to quickly and easily make changes. The use of thumbnails for researchers is nice as well. Perhaps even logo thumbnails could be added for the organizations table.
I think an easy way to see all of the data for every entry would be nice. The lists for each type (Researcher, Collaboration, or Organization) only contain the names. Some users may way to see every Researcher and their organizations in a single table, for example. Either an additional page is needed to display this information or the lists could contain all the data relevant to each object.

The system works quite well with a small amount of screen space. At 1/3 width and 1/3 height of my screen, I was still able to see the tables and all of the text boxes (on the add/edit pages).
One small change I would suggest is using a button instead of link for the "create" button after the tables. Using a button and perhaps placing an additional one just before the table could make it easier for new users.


Summary and Lessons Learned:

I learned a great deal from reviewing this project, since our project is less polished at this stage. While there are many components to this system, they are well integrated and nearly everything works perfectly. After reviewing this project, I may break up my project at this point to modularize it.
The login system using the SecurityFilter works very well. The login system used by my project is very different. Perhaps the SecurityFilter method is better.
I also realized how thorough the testing needs to be for this project. Since 1.2 this project has become much more complex and the volume and quality of test cases needs to reflect that. I will add many more tests to my own project now that I have seen many methods of testing this type of project.