Fixing What Ain't Broke

We rolled out a bunch of new code during the middle of this past week, and the change I posted contained these lines:

The following features have been rewritten but should behave identically to how they always have behaved. Please report any abnormalities with them:

1. all damage-over-time spells (necro spells, headache, poison, plague)
2. unholy fire, reflect (does anyone still have this spell?), and strawberry fulfillment

Upon reading this, a very astute player asked a very sensible question: "When you say DOTs have been rewritten but act the same, why rewrite them?"  After all, if the code ain't broke, why fix it?

While I cannot discuss the specific case of the recently rewritten DOT spells (I rewrote them as a part of a still-secret new feature), I'll use another recently rewritten spell to illustrate.  Back in early April I rewrote the mirror images/astral projection/shield of skulls spells.  Briefly, the reasoning was this:

1. We were going to make pinch always land.  This prompted a rewrite of pinch
2. Pinch is effectively a DOT spell; it wakes you up on the tick by dealing a small amount of damage to you
3. Pinch did this damage by calling the damage() routine, whose function is to apply damage to a victim after checking for sanctuary, magic ward, resists, vulns, and most importantly, mirror image
4. Since pinch did such little damage to begin with, it would sometimes not wake you up if you had sanc/ward/etc and they reduced pinch's damage to 0^*
5. I changed pinch from using damage() to raw_damage(), a routine that does damage but ignores most other factors like sanc, ward, and mirrors.
6. Because we still wanted pinch to hit mirrors like poison did, I had to add special code into the pinch spell to make it destroy images in the same way that it does from within the damage() routine

To achieve this, I had to figure out how the mirror images code in damage() actually works.  When I opened up the file, I found this:

if (IS_AFFECTED2(victim, AFF_MIRROR))
  {
    for (paf=victim->affected; paf != NULL; paf = paf->next)
      {
        if (paf->type == gsn_mirror)
          break;
      }
    if (paf)
      {
        imagehit = number_range(0,paf->modifier);
  
        /* mirrors exempt */
        if( dt == gsn_backstab ||
            dt == gsn_blister ||
            dt == gsn_decay ||
            dt == gsn_atrophy ||
            dt == gsn_wilt )
          {
            imagehit = 0;
          }
            
        if (imagehit != 0)
          {
            act("$n hits an image of $N",ch,0,victim,TO_NOTVICT);
            act("$n DESTROYS an image of you!",ch,0,victim,TO_VICT);
            act("You DESTROY an image of $n!",victim,0,ch,TO_VICT);
            paf->modifier--;
            if (paf->modifier == 0) 
              { 
                affect_strip(victim,gsn_mirror);
                REMOVE_BIT(victim->affected_by2, AFF_MIRROR);
              }
                
            return FALSE;                                                                        
          }
      }
    else
      {
        bug( "Damage: Mirror Image failure.", 0 );
      }
  }

In English, the code essentially does this:

1. Check to see if the target of the damage() is affected by mirror images.  If he is, 
2. Figure out how many images they have left.  If we can find this out,
3. Determine the % chance of this damage hitting a mirror image:
• if the damage is being done by backstab, blister, decay, atrophy, or fester, the chance of hitting an image is 0% (these skills/spells will never hit images)
• otherwise, the chance of hitting an image is N/(N+1), where N is the number of images the victim has left
4. If an image is hit,
1. Send messages to the victim, the attacker, and the rest of the room,
2. Reduce the number of images on the victim by one,
3. and if the number of images is now 0, strip off the aff entirely so the victim can re-cast them

While the above code certainly works, it was written in a painfully verbose manner.  Every aspect of mirror images' behavior is checked separately and there are four levels of nesting in the logic.  Because the code is so drawn out and wordy, it's difficult to just glance at the code and see exactly what factors into the mirror image calculation.

Trying to extend and modify code like this to add new features is similarly difficult; not only do you need to figure out how it works, but you need to figure out at what level in the logic the new feature needs to be added.  And wouldn't you know it, huge swaths of the game's code are written in a similar fashion.  Either a lot of code is used to accomplish something very little (which makes the code hard to maintain, extend, or modify), or too little code is used to accomplish something very big (in which case the code causes the game to crash whenever someone does something out of the ordinary).

I suspect that most of the game's past coders weren't thinking about how easy their code would be to maintain in five or ten years, and that mentality resulted in a lot of garbage piling up without anyone cleaning up^**.  Such an approach to code development is not sustainable though, and that approach usually leads up to a major breaking point.  In my mind, Dark Risings reached that breaking point right around the same time I took over as coder and the game was crashing on a daily basis.

Although I have no formal training in anything having to do with programming, computer science, IT, or anything like that, my programming mantra is to do things the "right" way rather than the "easy" way.  Given our game's code, this results in a significant amount of my time on the game being spent on code maintenance rather than development.  While I never go into the code with the intent of just rewriting things, it winds up happening when I need to modify or add a new spell or feature that is tied into a gnarly piece of code from years past.  What starts out as a five-minute change can turn into literally days of restructuring and testing.  And the real kicker here?  Provided I did a good job, nobody will ever notice that I did anything.

This is all part of the job involved in maintaining our Frankenstein code though, and I've put in my share of bad code too.  I just thought it might be insightful to get a peek at what coding at Dark Risings entails.

And for those who are interested, I replaced the old mirror image code (above) with this version which does the same thing in a much more concise fashion:

if ( IS_AFFECTED2(victim, AFF_MIRROR)
&&  (paf = affect_find(victim->affected, gsn_mirror))
&&  number_range(0,paf->modifier)
&&  dt != gsn_backstab
&&  dt != gsn_blister
&&  dt != gsn_decay
&&  dt != gsn_atrophy
&&  dt != gsn_wilt )
{
  act("$n hits an image of $N",ch,NULL,victim,TO_NOTVICT);
  act("$n DESTROYS an image of you!",ch,NULL,victim,TO_VICT);
  act("You DESTROY an image of $n!",victim,NULL,ch,TO_VICT);
  if ( --paf->modifier == 0 )
    affect_strip(victim,gsn_mirror);
  return FALSE;
}

Much simpler, right?

---

^* This is not the whole story, but it's close enough
^** Also not entirely true.  My predecessor, Kesavaram, did a lot of cleaning up.  As far as I can tell, nobody else did.