If I had known how much work it would turn out to be, Imight have chosen another first projecty. However it is done now, and it was a great learning experience for Java and VASSAL. I have checked it in under the branch pgeerkens, down in …/mapgrid.
HexGridNumberingX is ready for prime-time, as an extension of HexGridNumbering that:
Implements Oblique hex-grid numbering for all cases; and
Eliminates the earlier design choice to implement the pick-window for hexes as
a cleverly-crafted rectangle. Each hex now has a pick-window that exactly matches its interior.
HexGridNumberingX can also be added to a VASSAL module as a custom class, without
any need to edit the build file. This took some work, as the implementation of HexGridNumbering
was having its Configurer(), and hence its forceDraw(), method invoked before the private variable
grid had been defined by addToParent(). The work-around was to override Configurer(), invoking
super.Configurer() only if getGrid() returns non-null. This seems to work fine.
Part of why this was such a bugger is that there are unchecked references to ex.getMessage() that
were causing a Null Pointer Exception, erasing memory of the Null Pointer Exception that had
caused the Exception call. I will be looking for, and writing if I don’t find it, a function as
follows:
If I had known how much work it would turn out to be, Imight have chosen
another first projecty. However it is done now, and it was a great
learning experience for Java and VASSAL. I have checked it in under the
branch pgeerkens, down in …/mapgrid.
You didn’t commit what you intended to commit. 7570 doesn’t have any
code in it.
OK That should have resolved it. The little black star beside my folders has disappeared now.
Also, there is a comprehensive test bed included in the module for the use of anyone who wants to try it out. The old AH games all need oblique numbering.
OK That should have resolved it. The little black star beside my folders
has disappeared now.
Some initial comments on your code:
Don’t use
“” + foo
to convert foo to a String. Use String.valueOf(), it’s more efficient.
Don’t use ‘new Boolean()’, ever. There are only two possible Boolean
values, and instances of them already exist; creating more only makes
more work for the garbage collector. If you want to get a Boolean from
a String, use Boolean.valueOf().
Use prefix ++ and – unless you actually need the values returned
first.
Any variable which you’re not going to reassign should be final.
I’ll try to make a more thorough review soon.
Also, there is a comprehensive test bed included in the module for the
use of anyone who wants to try it out. The old AH games all need oblique
numbering.
Tests belong under the test/ hierarchy. (Note that this is not yet observed
everywhere—there are still some tests floating around under src/, which
still need to be moved.)
Thank you for the quick feedback. I have made the changes you requested, but need a little time to get my test bed working again - the test hierarchy needs to be tweaked a bit. I will upload again soon.
#3 may take a little getting used to, as I have a 20 year habit of writing i++ except when I absolutely require --i.
OK That should have resolved it. The little black star beside my folders
has disappeared now.
Some initial comments on your code:
Don’t use
“” + foo
to convert foo to a String. Use String.valueOf(), it’s more efficient.
Don’t use ‘new Boolean()’, ever. There are only two possible Boolean
values, and instances of them already exist; creating more only makes
more work for the garbage collector. If you want to get a Boolean from
a String, use Boolean.valueOf().
Use prefix ++ and – unless you actually need the values returned
first.
Any variable which you’re not going to reassign should be final.
I’ll try to make a more thorough review soon.
Also, there is a comprehensive test bed included in the module for the
use of anyone who wants to try it out. The old AH games all need oblique
numbering.
Tests belong under the test/ hierarchy. (Note that this is not yet observed
everywhere—there are still some tests floating around under src/, which
still need to be moved.)
It might be worth knowing that the classes for Oblique Hex grid numbering had already been written as custom class imports, they just had not been included with standard Vassal for whatever reason…
An interesting exercise might be to compare these already written classes with what you’ve come up with for fun and giggles. The two classes, HexGridNumbering.java and ObliqueHexGridNumbering.java can be found in the VQSL-src/trunk/afk directory in the repository
Yes, I actually started from Brent’s implementation of ObliqueHexGridNumbering in the Afrika Korps module. However his approach was the more practical one of tackling the multiple scenarios singly, as needed for each game, and Waterloo needed settings not available in the Afrika Korps implementation. Then I discovered some subtle bugs and short-cuts in the HexGridNumbering, followed by the inability to auto-load from the Editor, and with the goal of learning quickly more about both Java and VASSAL I accepted the challenge of producing a single class that implements every hex-grid numbering known to man (any Vulcans out there?). While I do not claim to have fully succeeded, the work is good and I know a lot more Java and VASSAL now.
I’ve run out of time this evening to finish reviewing your patch, unfortunately. Your code looks ok, on the whole, but what this has reminded me of is what an awful mess the existing grid code is generally. We had a discussion about this a few months ago:
Adding more grid code without refactoring sends us further down the rabbit hole. I think a large part of the difficulties you had in extending HexGridNumbering stem from it’s kitchen-sink design. This is one of those places where it would be relatively easy to disentangle all of the parts so that everything is coded to an interface and each class does one thing, and also write unit tests for them. Would you be interested in doing this if I helped you get started?
Okay, maybe Reversed can be left out of this chain, but that is still 384 cases to be checked on.
To be practical one would need an automated regression suite:
a) probably 6 different grids, to capture these:
____7) MaxRows odd/even
____8) Mox Columns odd/even
____9) MaxObliques odd/even (if oblique)
b) some means of efficiently selecting points to ensure coverage of this:
____minimum of 4 points, to have odd & even rows and columns checked
c) a test engine to run through these 64 cases for each grid, for the entire point set selected.
____1) Oblique/Standard
____2) H-Descending
____3) V-Descending
____4) Sideways
____5) Reversed
____6) Stagger (if Standard) or NW_SE (if oblique)
Then one outputs the entire set as a suite of reports, that can be text-differenced to the standard and manually checked set.
By checking in each small, self-contained, change independently, and running the test suite on each build, one can get an email each morning announcing whicn code changes broke which test-cases. Before one even has time to forget why one made the change.
Some work to set up, but it is a real charmer once set-up. I had the privilege of creating this for the report engine that I built in the '90’s.
By checking in each small, self-contained, change independently, and
running the test suite on each build, one can get an email each morning
announcing whicn code changes broke which test-cases. Before one even
has time to forget why one made the change.
Some work to set up, but it is a real charmer once set-up. I had the
privilege of creating this for the report engine that I built in the
'90’s.
It isn’t necessary to build a system like this from scratch. It already
exists. We have a testing framework (JUnit). If you’re using the Makefile
for building, you can run all of the tests using ‘make test’. There’s
some way to do that from Eclipse, as well, in case you’re using that. You
don’t need to wait for an automated system to notify you—you can run
the tests immediately.
There’s also Hudson, which is a continuous integration framework. It will
email you when tests fail. Right now, we’re using a Hudson instance that
Ken set up, but sometime in the new year we’ll have our own at vassalengine.org.
