Put Code Where It Belongs

Kingdoms of Arcania feedback has been coming in, and one of the requests was to turn off the banners for controlled pop centers. The banners will still be displayed in the pop center tool tips, but will not be rendered on the map itself. I decided that the boys should nap long enough today for me to implement this - it's a simple if statement to prevent the rendering in KOA, right?

Well, the solution is that simple, but the process turned into a game of Hunt the Wumpus. All the pop center rendering code is contained in one class... or so I thought. A quick read through the code turned up an icon rendering method that includes: if a banner exists, render it. I commented out the line and ran the game to see the effects. None.

Scratching my head, I took a look around, and realized this line controlled the tool tip banner rendering, but not the flags that are displayed on the map. Several frustrating minutes later, I found the following piece of code in an unrelated class:

if( actor instanceof PopCenter ) {
... draw Banners ...
}

After a few minutes studying the code, I was able to apply an Extract Method and Move Method refactoring (with a few other tweaks to make the refactorings possible). This is the frustrating part. A few seconds thought when this code was written could have saved me a great deal of precious time today.

I've been guilty of this type of code in the past. This will serve as a reminder to myself:

"Whenever you're tempted to add instanceof to the code, you're missing an opportunity to improve your design."

Flexible Persistence (Or: More PCType Woes)

Note to self: when designing a persistence mechanism for an application, make the underpinnings as flexible as possible. If the container doesn't impose any restrictions (Relational database, I'm looking at you), then don't arbitrarily create them.

Many of the Fall of Rome rules definitions are persisted to a flat file. There are several methods that talk to an Archive, asking it to load and store items into the flat file. To make the developer's life easier, the API allows me to ask for an int when I know it's an int, and ask for a String if I know the data type is a string. Unfortunately, the items written and read from the underlying file also encode the type of data. This makes life extremely compilicated when trying to change from a char (the old PopCenterType encoding) and a String (much handier for new pop center types, like Small Towns).

If I want to change how PopCenterTypes are archived, I shall have to hand edit every existing file where they are archived, introduce some method of changing them within the code itself, or altering the underlying persistence mechanism to be type agnostic. All three of these methods will require hand editing the files where the information is stored (and it's duplicated in several places - the master rules file and each individual game turn file).

I believe it will be possible to solve this by attempting to read items the new way, failing, and defaulting to the old way. That's a messy solution to a problem that never had to exist. The rules are stored in flat files. Flat files store bytes with no concept of data type. Lets store everything as a String. Strings can be manipulated to whatever is required by the internal application. It might not be as performant, but for a game where each turn is run every three days, adding an extra couple cycles to the loading time to improve developer flexibility seems like a fair trade.

Encapsulate - Please!

I just spent the last hours trolling through the Fall of Rome source encapsulating Pop Center types. The concept of pop center type was littered throughout the code as public characters - either 'C', 'T' or 'V'. This has made it exceedingly painful to introduce the new Kingdoms of Arcania pop center types, like the hamlet. The code never cares about the letters C, T or V - they care about what these letters represent. Unfortunately, the logic that applies to cities, towns and villages is now spread throughout the application instead of being encaplated in one centralised location.

This work has laid the foundation for proper Cities, Towns and Villages to come to life in the code. Once I manage to encapsulate the behavior, I will then be able to add the new pop center types.

For any software develoeprs reading this, please encapsulate your data. It makes life so much easier for the next person.

Copy / Paste Is Never a Good Idea

It's ironic that two days after I write a tirade on the dangers of duplicate code that I succumb to the temptation and am thoroughly punished for it.

Kingdoms of Arcania and Fall of Rome both have a list of pop centers that are used to initially populate the world. The basic concept is:

1) Read in the first n pop centers in the list.
2) Define the placement rules that those pop centers must adhere to.
3) Shuffle the list and randomly place the pop centers on the map.

The list of pop centers is shuffled to make random placement possible. Each pop center that gets placed has n-m possible locations, where n is the number of hexes, and m is the number of pop centers already placed. If we don't shuffle the list before creation, pop centers at the end of the list would have a much lower variance in position than those at the start.

In Fall of Rome, the first pop centers have a hex defined, and in Kingdoms of Arcania they don't. This is important later.

This section of code is very specific to the game being created. Fall of Rome rules are vastly different than those for Centurion. So, my first step in pop center placement was to copy the initial code from the Fall of Rome version and create a new Kingdoms of Arcania version I could properly modify. Duplication between the two versions could then be refactored, as I would have proper test cases around both by the time it was finished.

My first step was to create the new KOA method, then try to create a new game using the admin interface.

"Error creating new game"

Gee, that's helpful. What's wrong?

After digging into the code, I discovered that it was trying to place two pop centers in the same hex. That's odd, but not impossible given the randomization in placement, so I try again. It happens again. And again. Suddenly the problem is not looking so random.

Further digging leads me to discover that it was trying to place the same pop center twice.

Resisting the urge to add an if statement to avoid the case and get things working, I trace back the creation process and find from my copied / pasted code:

For first n pop centers in pop center list
Get info for new pop center
set up rules
create pop center
...

shuffleAndCreate pop center list

In order to guarentee the predetermined hexes could be used for the first Fall of Rome pop centers, they were created, and they were not part of the list that got added to the shuffleAndCreate method. My copy / pasted Kingdoms of Arcania code didn't make that distinction - it created the pop centers in order, then randomized the list and created them again, causing the aforementioned problem.

Copying and pasting code you haven't taken the time to understand is never, never a good idea. That way be dragons. My quick "lets bootstrap what I'm working on by copying the existing method" turned into a debugging session that wasted too much time.

All I can say is "I told you so!"