Review of Webspider-chiaofen-1.0.926

Review package: webspider-chiaofen-1.0.926, Package Author: ChiaFen Zielinski
1. Installation Review:
Installation is smooth. I did not encounter any problem when installing and trying to use this package.
Junit, PMD, Checkstyle, Findbugs are all passed with no errors or warnings.
Emma Coverage summary
class: 100% (1/1)
method: 100% (9/9)block: 93% (726/784)
line: 92% (140.1/152)
This is a good coverage.
ant -f dist.build.xml also BUILD SUCCESSFUL.
However, there is a small problem that when running java -jar webspider-chiaofen.jar-totallinks http://hackystat.org 100, It printed out plenty of empty lines. It may because of the logging scheme, which was implemented with println().

2. Code format and conventions review
Violations:

Files	Lines	Violation	Comments
WebSpiderExample.java	25-31	EJS-39	Document all members.
WebSpiderExample.java	294	EJS-32	Document is unrelative to the method, which is copy-paste from another method.
WebSpiderExample.java	241	EJS-33	Keep comments and code in sync.
WebSpiderExample.java	299	EJS-9	Use meaningful names.

3. Test case review

• Black box perspective.
  

traversePages(String urlLink, int numLink), findMostPopularPage() and findTotalPages(), these 3 method actually are not tested. The test method only call the main method once that will use these method and assert a meaningless assertion.

getNumLinks(), this method is tested properly.

isNum(String s), only method is called. Result is not verified properly.

logList (String urlLink, int numLink, String logging), same as above. But this is a method returns void, verify result may be difficult. However, I think do some digging into the data of the test object is able to verify as least a bit of the result, while author verify none.

• White box perspective.
  

I have to say that, the test case is terrible. It actually does no meaningful test. What it does is only to run the methods and see if there is any exception or error comes out. Nearly no result is tested. 5 of the 8 test method use assertEquals ("test", testUrl, testWebSpider.startUrl) to verify the result, which is meaningless in most of the test. The test pass even when I add some bug code in the class code. I think the only propose of this test case is to pass EMMA with a high coverage.

• Break da buggah.
  

When I run the program with arguments of -totallinks http://www.google.com, it ends with Array Index: java.lang.ArrayIndexOutOfBoundsException: 2.
When using -totallinks http://www.google.com 99999999999999999, it throw out a java.lang.NumberFormatException and break down.

4. Summary and Lessons Learned
We should really do out test right, use it to test our code, not to pass the QA tools.
My code does not do well when I test with the arguments above which cause problem. I didn't think of these things either. I only catch them and give out a warning. I think I can do it better by adding more useful statement that will help user use it right.