Well I basically spent the whole weekend hacking on the Dusk server code. A lot of refactoring and code cleaning, fixing messy logic, abstraction and information hiding. Getting rid of the nasty hungarian notation, moving to modern java types and constructs, and so on.
Broke a lot on the way, but managed to fix some of it and found a few bugs here and there too.
Some of the 'new' Java features that have been particularly handy:
- Switch on string. I didn't think much of the feature when I heard about it, but with some of the code having "symbol" tables with 50+ tests implemented using if(), the switch statement fit in very well. I did try converting the command parser to use a hash table but it required so many classes it blew out the code-size too much;
- Generic containers just to clean up a lot of unnecessary casts;
- The container classes other than "Vector". This replaced some custom container classes or eliminated a pile of code;
- Iterators. Let me do nice things with the map (more below);
- Subroutines. Quite a lot of code was just cut and pasted from place to place, and simply moving the snippets to parametrised functions just cleaned up a lot of clutter. Don't underestimate the power of subroutines;
- foreach came in handy in many places;
- inner classes allow for information hiding and better abstraction.
The Tile Map
So apart from just general code cleaning I also replaced big chunks of code with simpler or better implementations. One of the bigger pieces was the TileMap class.
Previously the code had a couple of 2D arrays to hold the tile numbers and the entities on the given tiles. Every bit of code (from anywhere in the class structure) that needed it just accessed it directly - sometimes with a lock, sometimes with the wrong lock, and sometimes with no lock at all. So apart from having a public array being accessed willy-nilly, most of the uses simply fell into the category of scanning the player-viewable area for information. i.e. ripe for code re-use.
As an aside, much of it just wasn't very well done code, it looks like whomever wrote it had some ArrayIndexOutOfBounds exception problems and kept poking it till it didn't crash then cut & pasted the result everywhere. For example:
int i,
i2,
i3,
i4;
DuskObject objStore;
i=0;
if (thnRefresh.intLocX-viewrange < 0) {
i = -1*(thnRefresh.intLocX-viewrange);
}
i2=0;
if (thnRefresh.intLocY-viewrange < 0) {
i2 = -1*(thnRefresh.intLocY-viewrange);
}
for (;i<mapsize;i++) {
if (thnRefresh.intLocX+i-viewrange < MapColumns) {
for (i3=i2;i3<mapsize;i3++) {
if (thnRefresh.intLocY+i3-viewrange < MapRows) {
objStore = objEntities[thnRefresh.intLocX+i-viewrange][thnRefresh.intLocY+i3-viewrange];
while (objStore != null) {
... process ...
objStore = objStore.objNext;
}
}
}
}
}
And this example is synchronized on this - which is the wrong lock.
So apart from the curious convention of post-fixing almost every reference with "Store", and giving loop indices meaningful names like 'i2', the logic is rather convoluted. Took me too much effort to realise it is a simple 2D scanning loop over (x-viewrange, x+viewrange) and (y-viewrange, y+viewrange) with some bounds checking; which can obviously go outside the loop.
As this was repeated in a few places I decided to encapsulate it into the TileMap and provide an Iterable interface that lets me share that logic. It also allows it to hide implementation details (e.g. rather than a 2d array, it is a 1D array - and it should probably just use a hash-table anyway) and handle locking reliably.
Of course the primary benefit is that it just simplifies every use:
for (TileMap.MapData md : map.range(refresh.x, refresh.y, viewrange)) {
for (DuskObject o : md.entities) {
... process ...
}
}
MapData includes both the tile info and the entities on that tile, and may in the future include per-tile scripts and so on (i'm not sure yet as the script stuff needs a context - which is currently the main game object). The object itself (MapData) and the list is only allocated once on the iterator, so it is cheap and relatively gc friendly to iterate.
But wait, there's more! There were two other main users of the map which required poking around it's internals: visibility testing (i.e. who can see what) and path finding. So I also implemented these inside the map, and took the opportunity to improve the algorithms (although they might need tweaking).
For looking I used Bresenham's algorithm for testing of obstructions through the line of sight. It does give 'enhanced vision' in that it will allow for looking diagonally through gaps which was not possible before, but that can be tweaked if it is a problem.
For path finding I used an implementation of A* (I started with this one) which is restricted to moves in cardinal directions. Since traverse-ability testing requires script execution a callback is used so the map code doesn't need to know about such things. Actually it is perhaps a bit too powerful - it will find a route to any tile you can see even if it has to traipse all the way around the map to get there. But again this can be tweaked without having to change every bit of code that uses it.
In both cases the meat of the code is somewhat smaller than that which it replaced and it is much easier to re-use, at only a modest cost of a bit of plumbing. e.g. a very simple example which prints the directions required to get from start to end, only moving on empty tiles:
MoveListener callback = new MoveListener() {
public boolean canMoveto(MapData md) {
return md.tile == 0;
}
};
for (TileMap.MoveData md : map.move(startx, starty, endx, endy, flags, callback)) {
System.out.println("Move: " + md.direction);
}
Next?
Well there is still on-going renaming, hiding, and abstraction work in the server. Most i/o isn't implemented reliably (e.g. delete file + write RandomAccessfile). The script compiler/executor uses non-symbolic integers as opcodes. There's too much use of a try-catch to hide bugs rather than fixing them. And there's some bugs in visibility syncing with the client which i've had troubles with - probably ripe for a complete re-do because after too many failed attempts at understanding the code, maybe it's the code at fault and not me.
As for the client, it still has some awt windows which need replacing. The shop and equipment screens are the primary ones, and it could always use an inventory screen as well. Once i've had enough of poking the server I might look at that. One problem on the client is that I fairly regularly get some exception inside javafx which stops it rendering - requiring a restart of the application. Seems to be a javafx bug on this system (i'm coding on my laptop, which I normally only use for C).
And for efficiency's sake at some point the server protocol should be converted to binary, or at least have a binary option. Well, actually a more robust protocol needs to be created.
Then at some point there will also be work on a code release, a couple of issues to sort out first and I think the client should be awt-free before then too. But i'll try to do it before I lose interest and find something better to occupy my time!
Update: Just found out i'm back to work in a week so I might get a bit more in while it's hot in my mind but I need to wind down to prepare for work again too.
