Deleting Code

I came upon an event handling function a year or two back. It was late, there was a crunch going on, I was tired as hell, and I needed to figure out what it did. Here’s something like it (or see the whole thing if you can’t read it in this format):

void EventHandler::handleEvent(const Vector& eventPos, const Transform& objectTrans, Object* sourceObject, int sourceId, float time)
{
    if (sourceObject && sourceId != -1)
    {
        Actor* sourceActor = getActorFromId(sourceId);
        if (sourceActor)
        {
            // Event was fired by an actor. Accumulate data in the actor's memory about how much firing strength it has fired.
            // Get information about source weapon.
 
            Object* actorVehicle = sourceActor->getPlayer()->getCurrentObject();
            Armaments* actorArms = actorVehicle ? (actorVehicle->getArmament()) : 0;
            int weaponId = actorArms ? actorArms->getActiveWeaponIndex() : -1;
            if (weaponId != -1)
            {
                if (actorArms)
                {
                    Weapon* weapon = actorArms->getWeapon(weaponId);
                    if (weapon)
                    {
//                        if (weapon->getExplosionRadius() > 0)
//                            sourceActor->getMemory()->setTimeOfEvent(time);
 
                        // Get information about target, and bots memory of target.
                        ObjectProxy currentTargetHandle = sourceActor->getMemory()->getTarget();
                        Object* currentTarget = currentTargetHandle.get();
 
                        // Always deal with primaryObject
                        if (currentTarget && !currentTarget->isPrimary())
                        {
                            currentTargetHandle = currentTarget->getPrimaryObject()->proxy();
                            currentTarget = currentTargetHandle.get();
                        }
 
                        const SensingData* currentTargetSensingData = sourceActor->getSenses()->getSensingData(currentTarget);
                        if (currentTarget && currentTargetSensingData)
                        {
                            ASSERT(0 <= weaponId && weaponId < 8, "Illegal weaponId: &u", weaponId);
                            //DEBUG_OUTPUT("FiringStrength accumulation for weaponId %u... %f\n", weaponId, cyrrentTargetSensingData->typesFired
 
[weaponId];
                            int targetType = currentTarget->getType();
                            if (targetType <= 3) // Lighter types
                            {
                                //for (int strType = targetType; strType <= 3; ++strType)
                                //    currentTargetSensingData->typesFired[weaponId] += weapon->getTypeValue(strType);
                            }
                            else if (targetType == 4) // Other type
                            {
                                //currentTargetSensingData->typesFired[weaponId] += weapon->getTypeValue(4);
                            }
                            else if (targetType == 5) // Third type
                            {
                                //currentTargetSensingData->typesFired[weaponId] += weapon->getTypeValue(2); // Other type
                                //currentTargetSensingData->typesFired[weaponId] += weapon->getTypeValue(3); // Other type
                                //currentTargetSensingData->typesFired[weaponId] += weapon->getTypeValue(5); // Third type
                                /*                                ObjectProxy victimHandle = targetObject->getVehicle(sourceActor->getTeam());
                                SensingData* spottedObj = sourceActor->getSensingDataFromHandle(victimHandle);
                                if (spottedObj)
                                {
                                spottedObj->fireSuccess += 1;
                                }*/
                            }
                            //DEBUG_OUTPUT("...became %f\n", currentTargetSensingData->typesFired[weaponId]);
                        }
                    }
                }
            }
        }
    }
}

There are lots of things I could say about the quality of the code… but it’s late, there’s a crunch, I’m tired and… what does the function do? I’ll let you think about that for a bit. You can safely assume that functions don’t have side effects.

Many people focus almost exclusively on writing new code. Some people even stay away from deleting code — instead simply commenting things out. As you see above, this makes the code extremely hard to read. Also, the commented-out code doesn’t work anymore. With other changes, the compiler no longer checks this code for errors (for instance, some of the commented-out functions had been removed entirely).

This also misses the entire purpose of having source control in the first place! If you want the code back, go check it out in perforce, subversion, cvs, or whatever source control system you’re using. If you’re not using a source control system… well you have bigger problems than commented code.

For all there is to be said about writing code, often the best thing you can do is delete some code. Delete the code you don’t need — delete the extra pieces that are in the way of working with the code efficiently.

I once took over ownership of a code base, and the first thing I did was strip nearly half of the code out in form of unused support code, unnecessary interfaces and adaptation code for layers that no one wanted or needed and code that was simply so bad quality it was better to rewrite than to maintain them.

So… what does the code do? Cookies for the first commenter with the right answer.

(Disclaimer: the code above is not exactly the code I found… but it matches in form, and is similar enough to make the point)

Update: Added a link to the code as a separate file so you can view it without the scrollbar headache.

6 Comments

  • By Rachel Blum, Wednesday, June 3, 2009 @ 20:12

    Hm. That looks straightforward. Nothing in if (currentTarget && currentTargetSensingData) does anything (assuming your asserts and get functions are side-effect free)

    That means you don’t need anything under “always deal with primary object”.

    It continues from there, so you have a large heap of do-nothing code 😉

    It’s always fun to encounter that… I really wonder what people think when they do this.

  • By Ken, Wednesday, June 3, 2009 @ 20:23

    It makes people scroll left-and-right a lot, even if they have a nice big widescreen display?

    Ha, just kidding. (But it does.) It ensures the weaponId is in the range [0,8).

  • By Sharkey, Wednesday, June 3, 2009 @ 20:36

    Assuming no side effects and that the assert is compiled out in a release build, this code does:

    Nothing.

  • By Ryan Graham, Wednesday, June 3, 2009 @ 20:36

    If none of the functions called have side effects and ASSERT() is a typical debug-only assert, the code does a whole lot of nothing other than exercising the methods used..

  • By slicedlime, Wednesday, June 3, 2009 @ 20:57

    Indeed you’re right… if you compile without the asserts, it’s the worlds longest NOP. That’s a lot easier to see now than at 11 PM on a crunch night though 😉

  • By James Aguilar, Thursday, June 4, 2009 @ 23:08

    This was very nice and your other articles seemed good too. I have subscribed to your newsletter.

Other Links to this Post

RSS feed for comments on this post. TrackBack URI

Leave a comment

WordPress Themes