Refactoring recipes: remove goto

As a result of my GW-BASIC to C# code generation project, I was left with a barely readable (though properly executing) program. Given its source material, it of course featured Dijkstra’s most maligned construct quite heavily — goto statements. Being a programmer indoctrinated during the last 20 or so years of computing, I was immediately compelled to get rid of them ASAP.

Many legacy code rescues begin with a golden master test. (I will channel Jay Bazuzi and point out here that tests are neither necessary nor sufficient for safe refactoring — but we won’t go there today.) So armed with such a golden master and acceptable code coverage, I got to it with the goto removal.

My first observation was that, like many other GW-BASIC programs of this type, the bulk of the gotos were targeted around the program’s main “loop” (using the term loosely). My second observation was that there were tons of other auxiliary gotos within the program body that jumped ahead to different “routines.” Essentially the structure was as follows:

        L90:
        // (... printing out some stuff ...)

        L100:
        // (... the "parser" ...)
        // (... followed by all the "verb routines" ...)

        // example "verb routine":
        L4800:
        ;

        if ((((verb.CompareTo("FIG")) != (0)) ? (-1) : (0)) != (0))
        {
            goto L4900;
        }

        if ((((noun.CompareTo("")) == (0)) ? (-1) : (0)) != (0))
        {
            PRINT(("") + ("WHOM DO YOU WANT TO FIGHT?"));
            goto L100;
        }

        if ((((noun.CompareTo("GUA")) != (0)) ? (-1) : (0)) != (0))
        {
            PRINT(("") + ("YOU CAN'T FIGHT HIM!"));
            goto L100;
        }

        if ((((currentRoom.CompareTo(16)) != (0)) ? (-1) : (0)) != (0))
        {
            PRINT(("") + ("THERE'S NO GUARD HERE!"));
            goto L100;
        }

        if ((((objectRooms[(int)(10)].CompareTo(-(1))) != (0)) ? (-1) : (0)) != (0))
        {
            PRINT(("") + ("YOU DON'T HAVE A WEAPON!"));
            goto L100;
        }

        PRINT(("") + ("THE GUARD, NOTICING YOUR SWORD,"));
        PRINT(("") + ("WISELY RETREATS INTO THE CASTLE."));
        map[(int)(16), (int)(0)] = (17);
        objectRooms[(int)(13)] = (0);
        goto L100;

Label L4800 is the beginning of the “Fight” routine. The guard clause at the beginning jumps to the next verb routine in the event that the verb did not apply — typical of a language like GW-BASIC where there is no real block form for the IF/THEN construct. The remaining statements make various comparisons and jump back to the beginning of the parser (L100) after taking the appropriate action. This brings us to my first goto removal recipe, in this simple case where only one label is involved (we are ignoring the guard clause for now, since it must be handled differently):

1. Start from the bottom after the last goto, and introduce an else block for the preceding if, completely enclosing all statements.
2. Continue moving upwards, changing if statements to else if until you reach last if containing the desired target label. At this point, the code is still functionally identical but retains the goto statements.
3. Introduce an unreachable goto to the target label following the final else.
4. Delete all preceding goto statements, making the final goto now reachable.

At this point, the code should look something like this:

        L4800:
        ;
        if ((((verb.CompareTo("FIG")) != (0)) ? (-1) : (0)) != (0))
        {
            goto L4900;
        }

        if ((((noun.CompareTo("")) == (0)) ? (-1) : (0)) != (0))
        {
            PRINT(("") + ("WHOM DO YOU WANT TO FIGHT?"));
        }
        else if ((((noun.CompareTo("GUA")) != (0)) ? (-1) : (0)) != (0))
        {
            PRINT(("") + ("YOU CAN'T FIGHT HIM!"));
        }
        else if ((((currentRoom.CompareTo(16)) != (0)) ? (-1) : (0)) != (0))
        {
            PRINT(("") + ("THERE'S NO GUARD HERE!"));
        }
        else if ((((objectRooms[(int)(10)].CompareTo(-(1))) != (0)) ? (-1) : (0)) != (0))
        {
            PRINT(("") + ("YOU DON'T HAVE A WEAPON!"));
        }
        else
        {
            PRINT(("") + ("THE GUARD, NOTICING YOUR SWORD,"));
            PRINT(("") + ("WISELY RETREATS INTO THE CASTLE."));
            map[(int)(16), (int)(0)] = (17);
            objectRooms[(int)(13)] = (0);
        }

        goto L100;

Now you are free to extract the code between the guard clause and the last goto as a separate method (which I did right afterward).

Many of the verb routines were structured just like this and were handled similarly. But there was one other common pattern, which involved two goto labels — specifically L90, which printed out information when you moved into a new room, and L100 which ran the parser. Here is one such routine which handles “Leave”:

        L4700:
        ;
        if ((((int)(((verb.CompareTo("LEA")) != (0)) ? (-1) : (0))) & ((int)(((verb.CompareTo("EXI")) != (0)) ? (-1) : (0)))) != (0))
        {
            goto L4800;
        }

        if ((((currentRoom.CompareTo(13)) != (0)) ? (-1) : (0)) != (0))
        {
            PRINT(("") + ("PLEASE GIVE A DIRECTION!"));
            goto L100;
        }

        if ((((int)(((noun.CompareTo("BOA")) != (0)) ? (-1) : (0))) & ((int)(((noun.CompareTo("")) != (0)) ? (-1) : (0)))) != (0))
        {
            PRINT(("") + ("HUH?"));
            goto L100;
        }

        currentRoom = ((objectRooms[(int)(11)]) - (128));
        goto L90;

There are two different labels here but we only need to modify the above recipe slightly to handle this. We’ll run all the same steps up to the point where we would introduce an unreachable goto. Instead:

1. Introduce a bool ret; declaration above the first refactored if. Keep it uninitialized, as we will need to lean on the compiler to help us avoid mistakes.
2. Below the last else block, add an unreachable block if (ret) { goto [label 1]; } followed by another statement goto [label 2];.
3. In all the places where goto [label 1] was used, replace it with ret = true;. Similarly, in all the places where goto [label 2]; was used, replace it with ret = false;. The last block is now reachable.

After this recipe, the code should look as follows:

        L4700:
        ;
        if ((((int)(((verb.CompareTo("LEA")) != (0)) ? (-1) : (0))) & ((int)(((verb.CompareTo("EXI")) != (0)) ? (-1) : (0)))) != (0))
        {
            goto L4800;
        }

        bool ret;
        if ((((currentRoom.CompareTo(13)) != (0)) ? (-1) : (0)) != (0))
        {
            PRINT(("") + ("PLEASE GIVE A DIRECTION!"));
            ret = false;
        }
        else if ((((int)(((noun.CompareTo("BOA")) != (0)) ? (-1) : (0))) & ((int)(((noun.CompareTo("")) != (0)) ? (-1) : (0)))) != (0))
        {
            PRINT(("") + ("HUH?"));
            ret = false;
        }
        else
        {
            currentRoom = ((objectRooms[(int)(11)]) - (128));
            ret = true;
        }

        if (ret)
        {
            goto L90;
        }

        goto L100;

Again, you should be able to easily extract the method at this point, get rid of ‘ret’ in favor of early returns, and inline the temporary ‘ret’ in the outer call.

The main point here is that even the ugliest code can be tamed with a series of disciplined refactoring steps. Tests are possibly nice to have but not actually required when the refactoring recipes you use are completely safe — like (I think?) the above ones are.