Crikey

I’m trying to refactor a delicate heap of code. Heap. Pile. Cesspool. Call it what you will. Its part of my TOE work.

So far its taken me two days and several restarts, to hack my way thru the weeds of poor nomenclature to discover that what I was dealing with was a very simple state engine that is driven by a modest little text file (.csv) interpreter.

The driver is actually very generic, but it clearly grew out of a specific use case, and the individual handlers have been copied and pasted, derived and adapted – some of the names are generic and some seem to be very specific. “count” turns out to be the field containg any general numeric value; when it was first used, the only numeric value was a counter.

It also suffers from opaque specialization – values passed as arguments to a function in some other file combined with hard-coded constants somewhere else combine to decide which columns will be in named fields while other columns somehow end up in an array called “values[]”. Some of these will be strings stored as values[n].tvalue, some will be integers stored as values[n].dvalue, and so on. You know which is which because you specify some of them and the engine works out the others for you.

The particular data being manipulated is the vehicle list. Did I say list? I mean’t lists. There are at least 110, each highly specialized.  And of course, with so many specialized lists, you need lists and structures to help you map between them.

What makes something like this take forever, however, is unwinding all the code that is there purely to make the code work. For instance, when I removed 2 of the lists, 24 other lists generated “unreferenced” warnings – they had existed purely to associate the first two lists, which neither the client nor host ever used.

I can see why it got convoluted: originally there was just the list of vehicles. Then it dawned on someone that you would have to limit different vehicles to different spawners — aircraft to airfields, for instance. So in addition, vehicles are organized into categories and categories can be attached to spawners and a spawnlist thus implied – the alternative being you would have to have a complete custom spawnlist for every facility on the map.

Which, infact, we do.

When I’ve gotten all this working, my next step is to approach these per-facility spawn lists; they exist because facilities had their own supplies. Now that they don’t, these lists can be simplified down to a list of actual spawner objects, and the spawn system can use the generic rules to eliminate vehicles that can’t be spawned — e.g. a Z34 destroyer can’t spawn from a riverine docks facility.

Then its time to try grafting & adapting the TOE code I’d developed into that codebase. I’m fairly sure that some of these weapon lists are to blame for the crashes I was getting with the development codebase. There were some very, very dubious string operations and some mallocs where perhaps callocs were intended…

13 Comments


…*twitch*…

Am I wrong in thinking that coming up with a more convoluted and inefficient way of doing things is something worthy of a Nobel Prize for Messing With People’s Heads?

Out of curiosity, you wouldn’t happen to have any documentation or notes from way back when to see what they were thinking, would you? (Of course, I wouldn’t be particularly surprised if there wasn’t any – if nothing else, the code definately points in that direction…)

Well – we do have documents that are supposed to do that… Specs and design docs and such.

There is old code that was written by Marty, which I can usually grok – Marty generally documents his exploitation of caveats and he was a stickler for reusing code (albeit by copy & paste). I can usually guess what he’s going to call a function.

But Marty’s coding practices had some quirks which rubbed against the other coders such that they missed out on his genius, and they are definitely programming in a different key.

So, there is the code by The Others which typically consists of things done in peculiar and unusual ways (circular linked lists, for instance). I think part of the problem is that Marty’s code and systems came with an amount of implementation overhead.

For instance, he used a C-based AVL system for ordered lists of data, which is basically an abstract system for managing data. This mean’t that every time the AVL library was used, you had to write a layer of code that sat between the AVL abstraction and the type-specific manipulation code.

Which meant that The Others generally tried to avoid using the AVL systems and implemented a custom list system for about half of the lists in the code.

When I was first digging around the code, I assumed these were for reasons of efficiency. A bad assumption like that can thwart your attempts to understand a piece of code.

For instance, the map/mission host had dozens of highly optimized custom list handlers with special key encoding/decoding functions, etc. And while efficiency may have been a goal of the programmer, once I got my head around the full set of lists I realized it wasn’t their imperative.

Example: there was a group of lists whose sole responsibility was to handle exceptions to a particular rule. There was no list of items that matched the rule, just lists of exceptions. However, there was no algorithm for determining what was or wasn’t an exception, so in all cases you would have to search through each exception list until you found a match or ran out of lists.

Each entry in the list had a key and a value. The key identified the case, the value would be either 1 (to confirm it is an exception) or 0 (which mean’t “not an exception” and could thus not occur except for a bug).

Since the lists were by design never intended to be accessed separately and by implementation impossible to access separately … It sort of seems as though the 1000+ lines of code that implemented the 5 different list systems used would have been better replaced with a single instace of 100+ lines of AVL wrapper code to create a single list where the value was an enumeration of the type of exception.

I’ve replaced all of the AVL code with Standard Template Library types which radically streamlines and simplifies huge tracts of source code.

But these custom lists and lists of lists are absolutely littered with boobytraps where some caveat of how the list is used or operated is critical to some other part of the system.

There aren’t many of them left, but the ones that are tend to be the ones that were hardest to extra in previous passes…

WOW, sounds like your having ‘fun’ there!

I think you may well be onto something about some of the older code causing your problems with your 1st TOE tests. If certain parts of the memeory handling was off it could certainly explain the odd errors that you where getting.

Anyway, glad to see your making progress. Keep up the good work.

Ahhh, Organic growth of code. It sounds more like each coder used the tool/method he was most familiar with and ran with it. Others came along and added on top of it.

Eventually someone has to rewrite it.

Congratulations! :)

Actually, I did quite a lot of work with organically grown software (I Y2K certified MMDF, for instance) prior to working here; there’s definitely a big difference when the growth is a gradual layering vs non-cooperation.

For instance, there is a distinct fingerprint of code Marty wrote that was replaced early on by one of the other coders, that code stands out because its usually functional overkill. I can’t distinguish where Marty had modified someone elses code during the early period, but there is a sudden transition to where you can tell post-layoffs Marty code and post-layoffs Marty fixes/refactorings of “other” code. There’s early kfsone code (which has elements of functional overkill) there’s Marty/kfsone code from the period where the two of us were working together, there’s post-Marty kfsone code and then there’s a final change from when Thunder was briefly working on the host and I took a final plunge towards using C++ and as much STL as possible.

So that leaves the question:

Will all this crap have to go over to the new engine as a complete rewrite? (Nightmares!)

Or will it transfer? (Sounds worse!)

Is it impossible to chuck this crap and start over? It seems you are making a completely new system anyway.

This was a good little module to attack – in my previous incarnations of TOEs I’d left this system intact, but its the base system for how the system knows about any type of vehicle. As I was going through removing the defunct references to the bits I’d removed, I was able to mark whole chunks of code as “///TODO:TOE:redundant This code … blah”

For instance, my last TOE system was relying on an adaptation of the supply system – facilities still had spawn lists. But that was only neccessary because the data wasn’t well coupled in the weap module (the one I just revamped) and coupling elements of the data was expensive.

For instance, in order to find the weapon definition for a player’s vehicle, so that it could tell if you were able to spawn that vehicle at the current facility never mind if the vehicle was available.

By refactoring both the code and the data, its now much, much simpler to ask “is this vehicle available at this facility” as well as to ask “which buildings can this vehicle spawn at”.

We organize our vehicles into categories — trucks, infantry, guns, etc, and these categories, rather than individual vehicles, are then associated with a spawner.

The current code goes off through a facility, checks every building in it for a spawn object, and then checks through a manually constructed replica of the category to see if *could* spawn your vehicle thru that building.

What I’m going to replace it with is quite simply a list of categories that the facility can spawn each of which will map to a list of the buildings that category can spawn from (e.g. some armybases have two barracks buildings, both of which can spawn the infantry category).

It’s not as tightly memory efficient as the current code, but it means that one data source can answer two questions: can my category spawn here or which buildings can my category spawn from — computationally the two are the same question.

Out of curiosity, you wouldn’t happen to have any documentation or notes from way back when to see what they were thinking, would you? (Of course, I wouldn’t be particularly surprised if there wasn’t any – if nothing else, the code definately points in that direction…)

HAHAHAHa! Hehehe, you really gave me the giggles there, Victarus. /me wipes tears from eyes.

Note to future dev teams:

1) when you’re creating everything from scratch, you always need a top-notch programmer that understands *all* the relevant systems: physics (if you’ve got them), networking, ui, databases, etc. This programmer may be very expensive but he’ll be worth it, even if only for the ability to call bullshit on all the other programmers when needed.
2) when programmers are antagonistic, all will suffer. Everyone needs to paddle in the same direction, or get out of the damn boat.

Juxst curious, but as a % of how much time this has taken you, how long would it have taken to do it all from scratch?

I remember that in the 6 months before and after the game was released, there were entire game systems that were produced. The development pace seemed very rapid. I wonder if it was because they had a clean slate to work with

(but of course, perhaps the work had some “quality” problems too!)

Trout

That’s hard to gauge. I honestly couldn’t tell you how long it would have taken to write what I’ve written from an absolute clean slate – meaning no game systems whatsoever.

Right now, I’ve made some good headway, but the trouble is that I have to keep stopping and studying code paths, classes, etc. It’s easy to get distracted by some little 10-liner class that performs an essential role to the current facility-base system, but that doesn’t need to be in the new system.

For instance, there is a class called WeaponEntry; it extends the basic class that we share client/host that describes any kind of weapon/vehicle. It adds some members that it might not hurt to move into the base class (the client won’t use them so they’ll be nothing more than a compile-time overhead). But there’s some bits and pieces I hadn’t factored into my hatchet job to the strat system.

Which means stopping and going and tracing all those code paths and determining what I can throw away and what I need to keep.

Invariably there’ll be some edge-case use of the class that it turns out to be something like early kfsone code where I was still learning all the classes, functions, etc, and I only knew about WeaponEntry and not the underlying class, so I used the higher level API uneccessarily. Or maybe there was some singular benefit to using WeaponEntry – perhaps it saved me having to use a second variable – and I can swap it out with a couple of replacement variables.

But then I have to descend all the codepaths from that function and check to see if that breaks somewhere.

Ouch, sounds like fun :)

One of the things I like about the new IDEs are that some have some nice tools to help in this regard, like telling you where in all your codebase is a certain method being called, renaming a variable/method/class changes the name in all locations… etc.

Not sure if there are IDEs with such features for C++, but they really help.

Good luck, this kind of “forensic refactoring” can be quite tricky, specially when you have code with hidden side effects and such, and it can be quite time consuming, as you have to modify, test you did not breack anything, rinse & repeat…

S!

Trackbacks and Pingbacks

Much better « kfsone’s pittanceMay 12, 2007 at 12:53 am

[…] Crikey […]

Leave a Reply

Name and email address are required. Your email address will not be published.

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

You may use these HTML tags and attributes:

<a href="" title="" rel=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <pre> <q cite=""> <s> <strike> <strong> 

%d bloggers like this: