Crying and complaining about coding

One of the hardest parts of coding for Dark Risings is the fact that it's derived from code that is over two decades old (Diku was first released in 1990) and, even after transforming into the ROM 2.4b6 codebase (released in 1998), it's been hacked on by twelve years of Dark Risings coders. As much as I hate to say it, the greatest amount of damage to the code has, in fact, been done by the long history of DR coders of varying levels of familiarity with both C and how ROM works.

What's prompted this whiny rant is a problem I discovered while generating a list of all of the scrolls in the game for Sidonie. I'd already done this with wands and staves with the help of a little Perl magic that I wrote, so I expected it to be another straightforward five-minute job. In a sense it was, but upon examining my data dump, I noticed that some scrolls had four spells (some of which are listed as "reserved") and others have blanks where spells should've been. Thinking that my Perl script was defective, I checked the area files themselves, and sure enough, some scrolls have "reserved" and others are just "". What's the difference? Since this sort of ill-defined behavior can lead to major issues (area/pfile corruption, crashes, etc), I started digging and was brought to one particular routine that made me a little crazy.

Consider the following code, straight out of the Dark Risings source:

/*
 * Lookup a skill by name.
 */
int skill_lookup( const char *name )
{
  int sn;

    for ( sn = 0; sn < MAX_SKILL; sn++ )
    {

      if( skill_table[ sn ].name == NULL )
        break;

      if( strcasecmp( name, skill_table[ sn ].name ) == 0 )
        return sn;

      /*
        if ( skill_table[sn].name == NULL )
            break;
        if ( LOWER(name[0]) == LOWER(skill_table[sn].name[0])
        &&   !str_prefix( name, skill_table[sn].name ) )
            return sn;
      */
    }
...

For whatever reason, a past coder decided it best to comment out the stock bits of this subroutine, presumably to force the subroutine to match the whole skill/spell name instead of allowing abbreviation (which can cause issues when you have spells like "lightning bolt" versus "lightning breath").

This sort of thing drives me crazy for many reasons, but here's one.

The stock version of the code was smart; it first checked the first character of the argument against the first character of the entry under examination in the master skill table. Since there's a very large chance that those first characters won't match when ripping through all 298 skills, you wind up not having to incur the overhead associated with a full-on subroutine call for the majority of the misses. This makes the whole process of finding a skill and returning its associated skill number much faster.

Perhaps I am more mindful of the benefit of these tiny performance benefits since I write very computationally intensive scientific code, where one poorly written line of code can add days or weeks onto compute time, for a living. And perhaps there really is no appreciable speed benefit to not preserving this preliminary first-character check when matching strings on modern hardware. However, I see little reason to pull it out outright since it is a smart way of handling these sorts of lookups.

Furthermore, the str_prefix routine (provided with ROM) was replaced with strcasecmp, which is intrinsic to string.h. However, whoever did this code change must have been extremely unfamiliar with ROM's internals (or perhaps extremely tired), because ROM provides str_cmp, its own equivalent to strcasecmp which is used extensively (and I mean extensively) throughout the code.

If strcasecmp and str_cmp serve the same purpose, why do both exist? As it turns out, strcasecmp is indeed included in string.h, but it is not ANSI C (although it is POSIX 2001). POSIX didn't exist back when Diku was first released (and neither did GCC or glibc for that matter), so it's likely that the str_cmp routine was written into the code decades ago. Providing this routine made the code more portable by not having to rely on special extensions that only existed in specific proprietary compilers. It was also tailored specifically to the needs of the code.

For the sake of consistency and portability, str_cmp (which is probably a little faster than strcasecmp) has been used exclusively in ROM's source. Unfortunately, DR's code has become rife with a sloppy mix of strcasecmp and str_cmp, forfeiting the benefits of portability while gaining literally nothing. As DR coders have touched the code, they've left these marks across it without really considering what the implications may have been.

True, since strcasecmp is now POSIX, it is unlikely that Dark Risings will ever be run on a system without it (e.g., DR compiles on HP-UX 11i, which predates POSIX 2001), but this is only an innocuous example. This sort of messy work, where one feature is implemented many times because various coders were ignorant of their predecessors' work, are all over the DR source, and it drives me a little nuts. I could go through and change all those strcasecmps to str_cmp, but to do so would require quite a bit of testing and would open the doors to new bugs. When it comes down to spending time cleaning up code which will not introduce any new features to the game or spending time adding new features while leaving the existing problems as they are, I find myself always choosing the latter.

Nobody cares if our code reads like garbage as long as that garbage is transparent to the players and imms, so I guess I'll have to take out my frustrations on this blog. It probably doesn't help that I spend my entire day at work cleaning up garbage FORTRAN.