I have some comments about revision 2785, merged from Michael’s branch.
Writing javadoc for new methods should be mandatory. It’s hard to
read other’s minds, or even one’s own mind from six months ago, and
VASSAL is quite large—I doubt that anyone is familiar with all of the
code. Adding two sentences explaining what a new method does will save
us a lot of time in the long run. (I also think we should add javadoc
for old methods lacking it—I try to do that whenever I make significant
changes to an old method.)
BadCoords: Should this be called BadCoordsException instead, in order
to follow the naming convention for Exceptions?
Local variables which are assigned to only once should be final, as
this lets the JVM do some optimizations which it otherwise can’t do,
and it might help you catch bugs.
In HexGridNumbering.getCenterPoint(int,int), there’s a line like this:
Are you really intending integer division here, or should that be “2.0”
instead?
In RegularGridNumbering, is it significantly slower to use an equivalent,
but much simpler regex like “-?([A-Z])\1*”? The one you have there is rather
baroque.
Please do not use StringBuffer unless you need the synchronization it
provides. StringBuilder is a drop-in replacement for StringBuffer, and is
up to twice as fast because it is not synchronized. (You have a StringBuilder
in RegularGridNumbering, and one in Zone.)
No worries, I just wanted to point this out. We have about 100k lines
of code right now, and a lot of it is poorly documented.
Yeah, this one isn’t your fault. I’d never noticed this before.
Yes. Any variable can be final. Any variable that can be made final
should be.
My mistake. grid.getHexSize() returns a double, so this is not integer
division—the “2” is promoted to “2.0”. Still, it would be better to
explicitly divide by “2.0” to avoid the implicit cast from int to double.
Hmm. Now I’m not sure I understand what’s going on here. What’s supposed
to be a valid hex location? Does ([A-Z])\1* cover all of the alphabetic
parts? Does [0-9]+ cover all of the numeric parts?
Are there actually any games where both coordinates are alphabetic?
I recently went through the whole codebase and uniformly replaced
StringBuffer with StringBuilder. I’ll change those two for you.
(This falls into the category of “don’t use things in the Java API
which are (nearly) deprecated”. Vector, Hashtable, and Enumeration
fall into that category, too. The more I look up things in the Java
API docs, the more things like this I find.)
Code which unpacks binary data tends toward the ugly. Mainly what’s
important here is documenting what the file format looks like—it
should be possible to understand the code once you understand the
file format. (The code that I wrote for unpacking Cyberboard GTLs
is like this as well.)
I spotted one other thing just now: In RegularGridNumbering, you have an assert(). It’s usually better to throw an IllegalStateException if this is testing for something that should never happen.
Hmm. I guess there’s some debate about assertions versus various
kinds of exceptions in Java. My C/C++ experience tells me that
assertions should be used for sanity checks… I’m rather conflicted
here.
I made a couple of minor modifications during the merge. First, Michael had removed the “throws BadCoords” from the MapGrid.getLocation() method signature, but I restored it in the name of staying compatible with customer module code (VASL had code that would probably break.) BadCoordsException would be a better name, but we’d have to keep BadCoords around as a deprecated class. The other change was in SendToDeck, where I made it an explicit choice to prompt for the destination deck and allowed the module designer to specify the prompt message.
I take a softer line on this. I think class-level comments are absolutely mandatory, but often find that well-named methods are fairly self-explanatory.
For asserts, I favor using them for sanity checks, where failure indicates a clear logic error in the code. Anything that might fail on bad input data should throw an exception instead.
True enough, but I’m a bit skeptical of comments that are basically
the same as the method name.
I’m trying to figure out what SVN has done to my copy of the trunk
directory. I merged the trunk into my directory which resulted in
some conflicts which I chose to resolve in favour of the archive.
However when I browse the repository, it appears as though that not
all the changes have been merged onto my computer. For example, if I
look at SetupStack.java on SVN the revision control tags say:
$Id: SetupStack.java 2785 2007-12-31 06:55:47Z rodneykinney $
but on my computer it still says:
$Id: SetupStack.java 2593 2007-11-16 14:26:27Z mkiefte $
When I look closer, it does look like Rodney changed a few things
(like the BadCoords exception), but SVN didn’t update it on my
computer. This has got to be obvious but what am I doing wrong?
