WebSpider Review

Today's assignment is WebSpider Review. I'm reviewing Ben Karsin's spider.

---

Installation

The package installed fine, including all the ant files. Everything verified and ant jar worked.

The one thing that gave me a little trouble was the logging. Running the program with -logging at the end of the command line didn't log anything. I was pretty confused by that until I checked the source code. All the logging code was just commented out.

Aside from that minor snag, all was well.

Code Format Fixes

All in WebSpider.java:

Line	Rule	Rule Description
13	ICS-SE-Java-4	Do not include // comments at the top of the file.
40	EJS-27	Pluralize the names of collection references.
152	EJS-36	Use standard comments to hide code without removing it.

Test Cases

Black Box

The testing wasn't really being done from a black box perspective. I think there was just a decision to go with white box.

Because of that, there isn't much partitioning into equivalence classes. The JUnit tests cover a typical case (starting at http://www.hackystat.org) and the case of a bad URL for the starting page (http://). They also cover both of the command line arguments.

Some equivalence classes that could be added:

• Pages that link to themselves.
• Cycles of more than one page.
• "Diamond" linking (A links to B and C, B and C both link to D).
• Pages with links to missing pages.
• Pages with bad URLs in their links.
• Pages with no links.
• Running out of pages before the maximum.

These would be tested mostly for the result rather than whether or not they throw an exception. The hackystat.org test probably already has several of these, but it's only being tested for exceptions.

White Box

Test coverage is much better from a white box perspective.

Emma reports code coverage of:

Emma Coverage summary
class:   100% (2/2)
method:  100% (7/7)
block:   95%  (346/364)
line:    94%  (68/72)

A couple of white box tests that could be added:

• Relative links.
• Duplicate links on the same page.

Some of the things listed under black box could also be seen as white box tests because there's specific code to handle them.

Breaking

Page with no links:

$ java -jar *jar -mostpopular 'http://www2.hawaii.edu/~jaress' 10
Exception in thread "main" java.lang.NullPointerException
       at edu.hawaii.webspider.WebSpider.main(WebSpider.java:133)

Lessons Learned

My spider actually has a very similar flaw:

$ java -jar *jar -mostpopular 'http://www2.hawaii.edu/~jaress' 10
Most Popular: http://www2.hawaii.edu/~jaress/ with null

(It should say "0" instead of "null".)

Probably the best way to fix that would be initialize the count for the start page to zero.

Seeing Ben's approach also helped me decide what I should have done differently. There were parts of my spider that I wasn't happy with, and I was thinking about how I should have done it. After seeing Ben's code I decided that the root problem was over-generalizing.

I had created a helper class that was designed as if I had no clue what it was actually going to be used for. For example, it would return objects defined in HttpUnit -- with all their error-prone methods -- in case the calling code wanted to do something wacky with it like analyze the text. It also forced the calling code to decide when it had seen enough pages, in case the decision needed to be based on some complicated logic.

That was a Bad Idea, and my helper should have:

• Pulled the link data out of the HttpUnit object, put it into a simpler object, and returned that.
• Kept track of the maximum number of pages.

WebSpider

Today's assignment is WebSpider. My spider is here.

---

All three tasks accomplished.

HttpUnit

I used HttpUnit to create fake servers for testing my spider, so I learned about the server parts of it as well as the client parts. It's a actually nicer on the server end, and I wouldn't mind using it for that again. (But it's terrible on the client side.)

Logging

I didn't spend very much time on logging. I just grabbed something from Logger.getLogger and used setUseParentHandlers to turn it on and off. That seems a bit crude, but it works.

Kevin English posted some nice code to suppress all those HttpUnit stack traces. It's not really a part of logging, but it cleared away the screen so I could see my log.

Java

I had a very interesting time with Java on this project. HttpUnit (on the client side) has a bad habit of declaring checked exceptions that it never throws, then throwing many different unchecked exceptions that crash the program.

At the same time, I was trying to wrap part of the program in an Iterator for the silly reason that it made logical sense. Java won't let you add checked exceptions when you're implementing an interface, so I couldn't just propagate the checked exceptions knowing they didn't actually happen.

I ended up with some dead code to satisfy the compiler, a lot of try/catch blocks covering different levels of nesting (if it fails in the outer loop, continue the outer loop; if it fails in the inner loop . . .) and pre-fetching to make sure that next always worked when hasNext was true.

Basically, several rules of Java that are supposed to stop things from going wrong actually helped them go wrong and then got in the way of fixing them.

Stack

Today's assignment is stack. My stack is here.

---

Results

All five tasks completed successfully.

Problems

I had the same problem that Randy had with eclipse not finding classes. Thanks to Randy, it was easy to fix.

Thanks also to Jianfei Liao for pointing out that we might be expected to explicitly check for an install of JavaNCSS, even though a proper check is redundant to trying to use it.

I added the check in case it's required, but I still think it's bad. That kind of redundant checking can confuse people.

The most interesting thing I did was for the code coverage. I did add unit tests, but I also changed some of the non-test code.

ClearStack had a method called getTop that did nothing except call the method top inherited from Stack. So I changed it to be a single method defined in Stack and used directly without re-definition in ClearStack (just like push and pop). I wasn't sure whether the combined function should be called top or getTop, but I decided that getTop was more conventional. Coverage improved significantly.

Then I added a test to make sure that getTop throws an exception if the stack is empty. I also expanded the "normal" test to check getTop on a one-item stack and Stack.toString on empty and three-item stacks. That brought it to 100%.

Ant

Ant is a nice tool, but I think it sometimes tries to do too much.

It also has few rough edges. For example, if you try to expand a property it doesn't recognize, it silently uses the unexpanded form. That gives you things like:

[echo] PMD found ${pmd.failure.count} problem(s). 

Standards

I understand the motivation for the build file structuring, but I'm not pleased with it. What I'd like to do (but didn't) is:

1. Rename build.xml to compile.build.xml (and update all the import references).
2. Rename verify.build.xml to build.xml.
3. Import dist into the new build.xml.

That wouldn't make any of the files harder to understand. In fact, the new build.xml would be easier to understand than the old one. But it would make everything a lot easier. You'd do ant to verify, ant dist to make a distribution, ant pmd to run just PMD, etc.

JavaNCSS vs. SCLC

I prefer JavaNCSS. It was easier to get useful information out of the JavaNCSS output. It only looks at the Java code, but it looks at it in much greater depth. The SCLC output left me wondering "OK, what do I do with that?" It's also nice to have the cyclomatic complexity.

The non-comment line counts don't match, and I think that JavaNCSS might be excluding blank lines while SCLC is including them. That would be another plus for JavaNCSS.

Code Ruler Redux

Today, we have Code Ruler Redux. My ruler was reviewed by Randy Cox, and I have a revised ruler here.

---

Team Member

Brian Jaress

Style Changes

First, of course, I fixed the class coding standards violations that Randy found.

I also changed some of the code to make better use of Java 5 features. In a couple places I'd been looping over an array and adding every element to a Set, and I changed it to a one-liner using Arrays.asList() and addAll().

Strategy Changes

I also made some adjustments to the strategy, adding a target minimum number of peasants and always attacking enemy knights when there are no other enemy pieces. That took care of two edge cases that came up surprisingly often.

Here are the updated test results against built-in rulers:

Migrating	jaress
1	773
0	781
0	783

Gang-up	jaress
176	668
162	701
80	732

Split-up	jaress
180	654
148	653
202	610

Shaoxuan Zhang, whose ruler I reviewed, checked the code of successful rulers from another school and adapted them into something that sounds similar in spirit to my strategy: figure out what types of targets you want to attack and have each knight attack the closest one.

It's not quite the same, though. His ruler attacks castles first, then attacks knights and peasants once it has all castles. My ruler attacks castles, peasants, and weak knights first. When all of those are gone, it attacks strong knights.

Lessons Learned

Arrays.asList() is a very handy function static method, and Eclipse is a lot easier now that I've hooked up my mouse.

I also realized that my ruler is evil.

Of course, all the rulers try to kill off enemy pieces and squeeze out other rulers, and my ruler isn't the most aggressive or effective. If, however, you think of the strategy as representing the behavior of an actual person, my ruler is the worst thug.

The other rulers seem to have either sophisticated strategies that optimize the local behavior of their pieces (Ben Karsin's ruler is a good example) or simple strategies like "capture the castles" or "split up into groups." My ruler's simple strategies are "target the weak" and "don't let anyone have a bigger army." Even the peasant strategy, which I think is quite effective, leads to a creepy "peasant Matrix' effect.

Code Ruler Review

Today's assignment is Code Ruler Review. I'm reviewing the code of Shaoxuan Zhang.

---

Overall Impressions

A little dense in places, but generally well written. The strategy was clear and sensibly implemented.

I noticed that English does not seem to be Shaoxuan's first language. I had no problem understanding the comments, but the type of grammar tweaking that the rules insist on seemed rather pointless. I think maybe those rules should be relaxed in this case (but I listed them anyway).

The number of things to fix was surprisingly low, considering it was written before we got the rules.

Standards Check

Entries in the "Lines" column refer to the MyRuler.java file.

Lines	Rule	Rule Description
	ICS-SE-Java-0	Follow EJS standards.
1	ICS-SE-Java-1	All code is within the package hierarchy "edu.hawaii".
1	ICS-SE-Java-2	Do not use the wildcard "*" in import statements.
	ICS-SE-Java-3	Only one Java statement per line.
	ICS-SE-Java-4	Do not include // comments at the top of the file.
	ICS-SE-Java-5	Rewrite default Eclipse template JavaDocs to be useful.
	ICS-SE-Java-6	Format JavaDoc summary lines correctly
	ICS-SE-Java-7	Do not use Vector and Hashtable classes.
	ICS-SE-Java-8	Do not use "raw" Collections classes
	ICS-SE-Java-9	Prefer the for-each control structure to the for control structure.
	ICS-SE-Java-10	Use the @Override annotation when overriding equals() and hashCode
	ICS-SE-Eclipse-1	Use Eclipse Europa 3.3
*	ICS-SE-Eclipse-2	Import and use the Eclipse Java Code formatting template.
	ICS-SE-Eclipse-3	Configure Text Editor.
	EJS-1	Adhere to the style of the original.
	EJS-2	Adhere to the Principle of Least Astonishment.
	EJS-3	Do it right the first time.
	EJS-4	Document any deviations.
	EJS-5	Indent nested code.
	EJS-6	Break up long lines.
	EJS-7	Include whitespace
22,23,24,*	EJS-8	Do not use "hard" tabs
	EJS-9	Use meaningful names.
	EJS-10	Use familiar names.
	EJS-11	Question excessively long names.
	EJS-12	Join the vowel generation.
	EJS-13	Capitalize only the first letter in acronyms.
	EJS-14	Do not use names that differ only in case.
	EJS-15	Use the reversed, lowercase name of your organization's Internet domain name as the root qualifier for your package names.
	EJS-16	Use a single, lowercase word as the root name of each package.
	EJS-17	NOT USED
	EJS-18	Capitalize the first letter of each word that appears in a class or interface name.
	EJS-19	Use nouns when naming classes.
	EJS-20	Pluralize the names of classes that group related attributes, static services or constants.
	EJS-21	Use nouns or adjectives when naming interfaces.
	EJS-22	Use lowercase for the first word and capitalize only the first letter of each subsequent word that appears in a method name.
157	EJS-23	Use verbs when naming methods.
	EJS-24	Follow the JavaBeans™ conventions for naming property and accessor methods.
	EJS-25	Use lowercase for the first word and capitalize only the first letter of each subsequent word that appears in a variable name.
	EJS-26	Use nouns to name fields.
	EJS-27	Pluralize the names of collection references.
	EJS-28	Establish and use a set of names for trivial "throwaway" variables.
92,96,100,*	EJS-29	Qualify field names with "this" to distinguish them from local variables.
	EJS-30	When a constructor or "set" assigns a parameter to a field, give that parameter the same name as the field.
	EJS-31	Use uppercase letters for each word and separate each pair of words with an underscore when naming constants.
	EJS-32	Write documentation for those who must use your code and those who must maintain it.
	EJS-33	Keep comments and code in sync.
12	EJS-34	Use the active voice and omit needless words.
22,28	EJS-35	Use documentation comments to describe the programming interface.
	EJS-36	Use standard comments to hide code without removing it.
32,36,40,*	EJS-37	Use one-line comments to explain implementation details.
	EJS-38	Describe the programming interface before you write the code.
91	EJS-39	Document public, protected, package, and private members.
	EJS-40	Provide a summary description and overview for each package.
	EJS-41	Provide a summary description and overview for each application or group of packages.
	EJS-42	Use a single consistent format and organization for all documentation comments.
	EJS-43	NOT USED
	EJS-44	Wrap code with <pre>...</pre> tags.
	EJS-45	Consider marking the first occurrence of an identifier with a {@link} tag.
	EJS-46	Establish and use a fixed ordering for Javadoc tags.
125,150,198,*	EJS-47	Write in the third-person narrative form.
8,124,149	EJS-48	Write summary descriptions that stand alone.
124,149	EJS-49	Omit the subject in summary descriptions of actions or services.
8	EJS-50	Omit the subject and the verb in summary descriptions of things.
10,152	EJS-51	Use "this" rather than "the" when referring to instances of the current class.
	EJS-52	Do not add parentheses to a method or constructor name unless you want to specify a particular signature.
	EJS-53	Provide a summary description for each class, interface, field and method.
91	EJS-54	Fully describe the signature of each method.
	EJS-55	Include examples.
	EJS-56	Document preconditions, postconditions, and invariant conditions.
	EJS-57	Document known defects and deficiencies.
	EJS-58	Document synchronization semantics.
	EJS-59	Add internal comments only if they will aid others in understanding your code.
	EJS-60	Describe why the code is doing what it does, not what the code is doing.
172,187,205,*	EJS-61	Avoid the use of end-line comments.
	EJS-62	Explain local variable creations with an end-line comment.
	EJS-63	Establish and use a set of keywords to flag unresolved issues.
	EJS-64	Label closing braces in highly nested control structures.
	EJS-65	Add a "fall-through" comment between two case labels, if no break statement separates those labels.
	EJS-66	Label empty statements.
	EJS-67	Consider declaring classes representing fundamental data types as final.
	EJS-68	Build concrete types from native types and other concrete types.
	EJS-69	Define small classes and methods.
	EJS-70	Define subclasses so they may be used anywhere there superclass may be used.
55,59,63,*	EJS-71	Make all fields private.
	EJS-72	Use polymorphism instead of instanceof.
	EJS-73	NOT USED?
	EJS-74	NOT USED
	EJS-75	Replace nontrivial expressions with equivalent methods.
105,109,132,*	EJS-76	Use block statements instead of expression statements in control flow constructs.
	EJS-77	Clarify the order of operations with parentheses.
	EJS-78	Always code a break statement in the last case of a switch statements.
	EJS-79	Use equals(), not ==, to test for equality of objects.
	EJS-80	Always construct objects in a valid state.
	EJS-81	Do not call nonfinal methods from within a constructor.
	EJS-82	Use nested constructors to eliminate redundant code.
	EJS-83	Use unchecked, run-time errors to report serious unexpected errors that may indicate an error in the program's logic.
	EJS-84	Use checked exceptions to report errors that may occur, however rarely, under normal program operation.
	EJS-85	Use return codes to report expected state changes.
	EJS-86	Only convert exceptions to add information.
	EJS-87	Do not silently absorb a run-time error exception.
	EJS-88	Use a finally block to release resources.
	EJS-89	Program by contract.
	EJS-90	NOT USED
	EJS-91	Use assertions to catch logic errors in your code.
	EJS-92	Use assertions to test pre- and post-conditions of a method.
	EJS-93	Use threads only where appropriate.
	EJS-94	Avoid synchronization.
	EJS-95	Use synchronized wrappers to provide synchronized interfaces.
	EJS-96	Do not synchronize an entire method if the method contains significant operations that do not need to be synchronized.
	EJS-97	Avoid unnecessary synchronization when reading or writing instance variables.
	EJS-98	Consider using notify() instead of notifyAll().
	EJS-99	Use the double-check pattern for synchronized initialization.
	EJS-100	Use lazy initialization.
	EJS-101	Avoid creating unnecessary objects.
	EJS-102	Reinitialize and reuse objects to avoid new object construction.
	EJS-103	Leave optimization for last.
	EJS-104	Place types that are commonly used, changed, and released together, or mutually dependent on each other into the same package.
	EJS-105	Isolate volatile classes and interfaces in a separates packages.
	EJS-106	Avoid making packages that are difficult to change dependent on packages that are easy to change.
	EJS-107	Maximize abstraction to maximize stability.
	EJS-108	Capture high-level design and architecture as stable abstractions organized into stable packages.