Standards in Coding and Documentation: CodeRuler Peer Review

We have been learning about standards in coding and documentation lately in class. Standards improve readability, understandability and maintainability of code, and help others adapt and extend our code more easily. As an illustration of the importance of standards, and as practice in reading other people's code, we were assigned to review code written by our fellow students. For this assignment, I reviewed the CodeRuler implementation of Tyler Wolff and Daniel Tian.

Performance

The Wolff-Tian CodeRuler implementation performs very well against other rulers. It easily wins against all of the built-in sample rulers in one-vs-one combat, and it consistently defeats the Gang Up Ruler and the Smart Split Up Ruler combined. Mr. Wolff writes that "even in a 6 player free for all match our ruler wins 70% of the time," and I do not doubt this claim. I did not experience the disqualification for exceeding the 0.5 second time limit that was seen the other day when their implementation was demonstrated in class.

Documentation

The class is documented pretty well in the Javadoc comments. There is an overview description for the class, as well as an explanation of what each method does, how it works, and even some implementation details. For each method, there is also a list of what parameters it takes and what it returns. Some of the descriptions are a bit vague, however. For example, the summary for the orderSubjects method -- "This method is used by the CodeRuler application itself." -- is uninformative and could be removed without any loss of information. Another example is the summary for the movePeasant method: "Decides the next move for the specified peasant." This description doesn't tell me anything that I couldn't already infer from the name of the method.

Structure

The class is structured logically and cleanly. Methods are organized loosely according to the order in which they are called, and methods that perform similar functions are grouped together.

Logic

I like Tyler and Daniel's method of finding the closest enemy object. In the CodeRuler implementation that Aric and I worked on, we stored enemy knights in a TreeMap, with their distances as keys. Since TreeMaps are sorted by key, we then called firstKey to retrieve the nearest enemy knight. However, since we were only concerned with the nearest enemy knight, the rest of the TreeMap is wasted. Tyler and Daniel's method is much simpler.

There is a fair amount of duplication in their code. The findClosestEnemyKnight, findClosestEnemyCastle, and findClosestEnemyPeasant methods could all be replaced by a single, general findClosestEnemy method that takes an IObject array as a parameter; ICastle, IKnight and IPeasant arrays could still be passed to this method, and it would work just the same. Also, the movePeasant method is a long chain of if, else if, and else statements that could probably be replaced by a loop.

Standards

In this course, we will be following the conventions outlined in Elements of Java Style, along with some course-specific supplemental standards.

The table below lists deviations from these standards found in the code. In the violation column, EJS-# represents the rule number in Elements of Java Style that is being violated, and ICS-SE-Java-# represent the rule number in our course-specific standards that is being violated. Only the first three occurrences of a violation are listed; further violations are denoted with an asterisk (*).

File	Lines	Violation	Comments
MyRuler.java	N/A	ICS-SE-Java-1	Code is not within the package hierarchy "edu.hawaii"
MyRuler.java	1	ICS-SE-Java-2	Do not use the wildcard "*" in import statements
MyRuler.java	7	EJS-50	Omit subject (and verb) in summary descriptions of things
MyRuler.java	51	??	Non-Javadoc documentation
MyRuler.java	57	EJS-71	Make all fields private
MyRuler.java	70, 183, 247, *	EJS-49	Omit subject in summary descriptions of actions
MyRuler.java	174, 354, 424, *	EJS-29	The field variable rand is used without "this" to distinguish it from local variables
MyRuler.java	211, 212, 342, *	EJS-61	In-line comments at the end of an already long line
MyRuler.java	240, 365, 428, *	EJS-5	Opening curly brace should be on the same line as else
MyRuler.java	487	EJS-9	Be more explicit about what 35 and 31 represent by using named constants

Conclusions

Tracing other people's code can be confusing, even for a simple program like a CodeRuler implementation. Programmers can make it easier for other people to understand their code by providing comments and adopting standard procedures. Following a standard for coding and documentation can be tedious, however, and I am glad that there are templates and tools to help programmers do this.