Friday, February 02, 2007

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!"

2 Comments:

At 10:25 AM, Blogger Rick McDowell said...

Why did the code not create errors in Fall of Rome, but did so in Kingdoms of Arcania? Is it because the PC seeding rules changed?

 
At 3:24 PM, Blogger carlhume said...

Sort of. Fall of Rome doesn't add all the PopCenters to the shuffle list. It adds pop centers that required random placement to that list.

When I copied and pasted the code to create pop centers, I missed that pop centers with a location should not be part of that list - this resulted in them being created initially, and then created again in the same location.

 

Post a Comment

<< Home