17
\$\begingroup\$

EDIT_START: I want to thank all people for giving me such good answers! It's hard for me to choose any answer over another, because I see that all of your answers are valid and good in their own perspective. I want to clarify my own question. My question is not "How do I not use GOTO?", but my question is "How do I use GOTO in a better way?". This implicates that I want to use GOTO for program room/state transition at all costs. This is for educational purpose and for discovering the limits of C. I will give out a bounty as soon as possible to my question, to give back a reward. Anyway thank you all! I'll place a LABEL for ya all in my program ;-) EDIT_END:

I was discussing with someone about using GOTO at stackoverflow. May someone teach me some hidden tricks in using GOTO? Do you have some suggestions for improvement? You may enjoy my little adventure game, give it a try. ^^

PS play the game before you read the source, otherwise you get spoiled

#include <stdio.h>
#include <stdlib.h>

enum _directions{
    DIR_0 =    0b0000,
    DIR_E =    0b0001,
    DIR_W =    0b0010,
    DIR_WE =   0b0011,
    DIR_S =    0b0100,
    DIR_SE =   0b0101,
    DIR_SW =   0b0110,
    DIR_SWE =  0b0111,
    DIR_N =    0b1000,
    DIR_NE =   0b1001,
    DIR_NW =   0b1010,
    DIR_NWE =  0b1011,
    DIR_NS =   0b1100,
    DIR_NSE =  0b1101,
    DIR_NSW =  0b1110,
    DIR_NSWE = 0b1111
} DIRECTIONS;

void giveline(){
    printf("--------------------------------------------------------------------------------\n");
}

void where(int room, unsigned char dir){
    printf("\nYou are in room %i. Where do you want GOTO?\n", room);
    if(dir & 8) printf("NORTH: W\n");
    else printf(".\n");
    if(dir & 4) printf("SOUTH: S\n");
    else printf(".\n");
    if(dir & 2) printf("WEST:  A\n");
    else printf(".\n");
    if(dir & 1) printf("EAST:  D\n");
    else printf(".\n");
}

char getdir(){
    char c = getchar();
    switch(c){
        case 'w' :
        case 'W' :
            return 'N';
        case 's' :
        case 'S' :
            return 'S';
        case 'a' :
        case 'A' :
            return 'W';
        case 'd' :
        case 'D' :
            return 'E';
        case '\e' :
            return 0;
    }
    return -1;
}

int main(int argc, char *argv[]){
    START:
    printf("THE EVIL GOTO DUNGEON\n");
    printf("---------------------\n");
    printf("\nPress a direction key \"W, A, S, D\" followed with 'ENTER' for moving.\n\n");
    char dir = -1;
        
    ROOM1:
    giveline();
    printf("Somehow you've managed to wake up at this place. You see a LABEL on the wall.\n");
    printf("\"Do you know what's more evil than an EVIL GOTO DUNGEON?\"\n");
    printf("You're wondering what this cryptic message means.\n");
    where(1, DIR_SE);
    do{
        dir = getdir();
        if(dir == 'S') goto ROOM4;
        if(dir == 'E') goto ROOM2;
    }while(dir);
    goto END;
    
    ROOM2:
    giveline();
    printf("Besides another LABEL, this room is empty.\n");
    printf("\"Let's play a game!\"\n");
    where(2, DIR_W);
    do{
        dir = getdir();
        if(dir == 'W') goto ROOM1;
    }while(dir);
    goto END;
    
    ROOM3:
    giveline();
    printf("Man, dead ends are boring.\n");
    printf("Why can't I escape this nightmare?\n");
    where(3, DIR_S);
    do{
        dir = getdir();
        if(dir == 'S') goto ROOM6;
    }while(dir);
    goto END;
    
    ROOM4:
    giveline();
    printf("Is this a real place, or just fantasy?\n");
    printf("\"All good things come in three GOTOs.\"\n");
    where(4, DIR_NSE);
    do{
        dir = getdir();
        if(dir == 'N') goto ROOM1;
        if(dir == 'S') goto ROOM7;
        if(dir == 'E') goto ROOM5;
    }while(dir);
    goto END;
    
    ROOM5:
    giveline();
    printf("This is a big river crossing. I guess I need to JUMP.\n");
    where(5, DIR_SWE);
    do{
        dir = getdir();
        if(dir == 'S') goto ROOM8;
        if(dir == 'W') goto ROOM4;
        if(dir == 'E') goto ROOM6;
    }while(dir);
    goto END;
    
    ROOM6:
    giveline();
    printf("This place doesn't look very promising.\n");
    where(6, DIR_NSW);
    do{
        dir = getdir();
        if(dir == 'N') goto ROOM3;
        if(dir == 'S') goto ROOM9;
        if(dir == 'W') goto ROOM5;
    }while(dir);
    goto END;
    
    ROOM7:
    giveline();
    printf("\"Give a man a LOOP and you feed him FOR a WHILE;\n");
    printf(" teach a man a GOTO and you feed him for a RUNTIME.\"\n");
    where(7, DIR_NE);
    do{
        dir = getdir();
        if(dir == 'N') goto ROOM4;
        if(dir == 'E') goto ROOM8;
    }while(dir);
    goto END;
    
    ROOM8:
    giveline();
    printf("This looks like an endless LOOP of rooms.\n");
    where(8, DIR_NW);
    do{
        dir = getdir();
        if(dir == 'N') goto ROOM5;
        if(dir == 'W') goto ROOM7;
    }while(dir);
    goto END;
    
    ROOM9:
    giveline();
    printf("You've found your old friend Domino. He doesn't looks scared, like you do.\n");
    printf("\n\"Listen my friend,\n");
    printf(" If you want to escape this place, you need to find the ESCAPE KEY.\"\n");
    printf("\nWhat does this mean?\n");
    where(9, DIR_N);
    do{
        dir = getdir();
        if(dir == 'N') goto ROOM6;
    }while(dir);
    goto END;
    
    printf("You never saw me.\n");
    
    END:
    giveline();
    printf("The End\n");
    return 0;
}
\$\endgroup\$
  • 17
    \$\begingroup\$ If this program is an attempt to produce evidence of justified goto use, I think it's missed the mark. \$\endgroup\$ – Reinderien Oct 14 at 13:54
  • 18
    \$\begingroup\$ I'm honestly not sure what you're looking for. If you come to a code review forum, which emphasizes best practices, and ask how do I do this bad thing in a clever manner?, people are probably still just going to tell you not to do the bad thing. \$\endgroup\$ – Reinderien Oct 14 at 14:20
  • 7
    \$\begingroup\$ Please do not edit the question, especially the code after an answer has been posted. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered \$\endgroup\$ – pacmaninbw Oct 14 at 15:34
  • 13
    \$\begingroup\$ @paladin - goto-ing to code in another function isn't unpredictable, it's explicitly illegal. Labels have function scope, so they aren't visible outside the function where they're defined (just like local variables). If you want to jump between functions you'll have to use setjmp()/longjmp(), but those are very good ways to screw up your stack and crash your program in new and interesting ways. \$\endgroup\$ – bta Oct 15 at 1:29
  • 7
    \$\begingroup\$ There are a handful of good uses of goto in C, but it is not a good general-purpose mechanism for branching. Consider that in contrast, Java reserved goto as a language keyword, but never implemented it, and it isn't particularly missed there. What relevant features does Java have that C does not? Mostly it's labeled break and continue statements and exceptions. Good uses of goto in C are pretty much all for situations that would most likely be addressed by one of those features instead in Java. \$\endgroup\$ – John Bollinger Oct 16 at 17:40

10 Answers 10

4
+100
\$\begingroup\$

May someone teach me some hidden tricks in using GOTO?

goto is kind of a one trick pony. But is part of a family of control transfer constructs. All the looping and decision structures are in a sense a refined or specialized application of a goto. So from that pov, using goto is advisable only if it cannot be done with any of the "normal" control structures like if, while, for etc.

The next level up is function calls. A function call is a super-powered goto. Not only can you jump and execute a named piece of code, but you can also jump back right where you came from. Plus you can pass arguments and return a value, if desired.

Another level up is making use of function pointers. A pointer to a function can be saved in a variable or an array. It can be passed to and returned from functions. For a state machine like in the question, I'd be very tempted to organize the rooms into an array of function pointers. But I'd probably use a lot of macros, too.

The next level up from functions is setjmp/longjmp. These let you jump back across several levels of the call stack. It's sometimes useful to have a setjmp call in the main loop or initialization of the program and then the program can restart or bail-out if it runs into certain recoverable errors.

I suppose the next level up might be signal handlers and/or forking off child processes. Or maybe loading a dynamic library.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ I wouldn't recommend anyone to use setjmp for any purpose. It has all the problems of goto but also comes with lots of other plain dangerous undefined behavior too. A lot of these constructs are obsolete crap from ancient Unix days when dinosaurs walked the earth. \$\endgroup\$ – Lundin Oct 15 at 14:06
  • 2
    \$\begingroup\$ Even when I also see the dangers of those tools, I would like to thank luser droog anyway for his summarizing. I started to learn programming with Java, a "safe hazard free" environment. For some time I'm exploring C and its capacities. I'm interested also in using the dangerous things, more from a philosophical perspective than from a real "do I want to use this?" perspective. To see what's possible. I'm not an experienced programmer, but I've the opinion, that it's always wise to know about all opportunities. I really enjoy to discover those opportunities and I'm eager to learn how to use. \$\endgroup\$ – paladin Oct 15 at 16:29
  • 2
    \$\begingroup\$ @paladin If you want to explore "dangerous things" and get close to the metal, I recommend learning assembly. If you're writing in C, you want to avoid the "dangerous things" and do things the correct, idiomatic way. Pretty much anything is "possible", like blowing off your foot with a shotgun. I don't recommend trying it, though. \$\endgroup\$ – Cody Gray Oct 17 at 8:29
  • \$\begingroup\$ Excellent answer. \$\endgroup\$ – pacmaninbw Oct 17 at 11:33
  • \$\begingroup\$ @paladin I'm interested also in using the dangerous things, more from a philosophical perspective than from a real "do I want to use this?" Careful here code review is for code working as expected and your code does work, but philosophical perspective makes this too hypothetical and opinion based. Hypothetical questions are off-topic on Code Review and Opinion Based questions are off-topic on many of the stack exchange sites. \$\endgroup\$ – pacmaninbw Oct 17 at 11:37
21
\$\begingroup\$

The code you've written is more or less a state machine, written the way that one might be constructed in assembly language. A technique like that technically works, but it doesn't scale well and you can wind up with problems that are extremely hard to debug. Your code only needs a small tweak to use the more traditional C-language way to implement a state machine, which is easier to read, maintain, and debug.

int main(int argc, char *argv[])
{
    int state = START;
    char dir = -1;
    while(1)
    {
        switch (state)
        {
            case START:
                printf("THE EVIL GOTO DUNGEON\n");
                printf("---------------------\n");
                printf("\nPress a direction key \"W, A, S, D\" followed with 'ENTER' for moving.\n\n");
                state = ROOM1;
                break;
                
            case ROOM1:
                giveline();
                printf("Somehow you've managed to wake up at this place. You see a LABEL on the wall.\n");
                printf("\"Do you know what's more evil than an EVIL GOTO DUNGEON?\"\n");
                printf("You're wondering what this cryptic message means.\n");
                where(1, DIR_SE);
                do{
                    dir = getdir();
                    if(dir == 'S') { state = ROOM4; break; }
                    if(dir == 'E') { state = ROOM2; break; }
                }while(dir);
                break;
            
            case ROOM2:
                giveline();
                printf("Besides another LABEL, this room is empty.\n");
                printf("\"Let's play a game!\"\n");
                where(2, DIR_W);
                do{
                    dir = getdir();
                    if(dir == 'W') { state = ROOM1; break; }
                }while(dir);
                break;
            
            ...
            
            case END:
                giveline();
                printf("The End\n");
                return 0;
        }
    }
}

The code is mostly the same as before, with only a few small tweaks:

  • Added the enclosing loop and switch
  • Changed labels from ROOMX: to case ROOMX:
  • Changed jumps from goto ROOMX; to state = ROOMX; break;
  • Defined constants for START, ROOMX, etc (not shown)

Structuring your code in this way makes it more readable and avoids a lot of problems that goto spaghetti can have. It's a lot easier to ensure that you don't unintentionally fall through from one room's code into the next one (if you somehow bypass the code that sets a new state, you simply stay in the same room and try again). You also avoid many limitations of goto, like the inability to "jump over" the declaration of a variable-length array (see example 2 in section 6.8.6.1 of the C99 language spec). You can also add an explicit default case to intelligently handle any unexpected or erroneous room selections.

This type of structure also opens up all sorts of avenues for improvement. You could take the contents of each case and wrap it up in a function, and each case could be simplified to case ROOMX: state = do_roomx(); break;. With each room's code encapsulated, you could unit test rooms individually.

You could also notice that each room's code follows a predictable sequence (giveline() -> print description -> where() -> read input -> select next room) and write a generic function do_room(struct room_data* room) that could handle any arbitrary room. You'd then create a data structure struct room_data that holds all the information needed for each room (description text, movement directions, where each exit leads, etc). This would be more similar to how a game engine works. Your code would become shorter and more generic, and each individual room would be implemented as data instead of code. You could even store the room data in an external file, and then you'd have a generic game engine that you wouldn't have to recompile every time you wanted to modify your maze.


Asking "How do I use GOTO in a better way?" is akin to asking "How can I punch myself in the face in a better way?" The answer is: You don't. Using goto like you're doing is incompatible with any definition of the word "better" that I'm aware of. You're taking a construct that C handles natively (a switch block) and have re-implemented it using explicit jumps. You get less functionality and more potential problems. The only way to approach "better" is to drop the unnecessary gotos.

Remember that the C language is just a thin, portable veneer on top of assembly language. goto is a wrapper around your CPU's "jump" instruction. None of the CPUs that I know of have instructions comparable to things like switch or for. These are syntactic sugar that the compiler rewrites for you into a sequence powered by "jump" instructions. For example, a simple loop like this:

for (i = 0; i < limit; i++)
{
    ... code ...
}

is treated as if it was written like this:

    i = 0;
LOOP_START:
    if (!(i < limit))
        goto LOOP_END;
    ... code ...
LOOP_CONTINUE:
    i++;
    goto LOOP_START;
LOOP_END:

A continue statement would be equivalent to goto LOOP_CONTINUE, and a break statement would be equivalent to goto LOOP_END.

A switch block is implemented similarly. Each case is a block of code with a label, and the switch jumps to a label based on the input value. break jumps to the end. This is generally similar to the way that you wrote your code. The key difference is that a switch block doesn't jump directly between cases. If you want to execute more than one case, you use a loop to run the switch block more than once.

Ultimately, the switch version and the goto version likely look almost identical once they're compiled. When you use switch, you give the compiler the chance to avoid certain problems such as ensuring that you're not jumping into the scope of a local variable while skipping over its initializer. When you write the goto-based version, the compiler will compile your code as written and trust that you know what you're doing. If you insist on using goto explicitly, you'll end up running into the sort of problems that led people to invent things like switch in the first place. When using goto "at all costs", those costs are frequently a program that behaves inconsistently and unpredictably. If you're looking for advice on how to program poorly, you're in the wrong place.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ State machines like these are not using goto but they are still spaghetti programming. Particularly nasty spaghetti if you need each state to become a function, it quickly turns into an unreadable mess. \$\endgroup\$ – Lundin Oct 15 at 6:45
  • 2
    \$\begingroup\$ @Lundin, I strongly disagree. State machines are the right tool for many important jobs: parsing regular languages, designing event-driven systems like UIs, modeling concurrent systems, and more. (Hat tip Devender Mishra and AJed.) \$\endgroup\$ – Vectornaut Oct 15 at 13:49
  • 1
    \$\begingroup\$ @Vectornaut I didn't argue against the use of state machines, I argued against the use of "stateghetti" where each state jumps around to other states in a decentralized manner. I just posted a version with properly written, centralized decision making here. \$\endgroup\$ – Lundin Oct 15 at 14:14
  • 2
    \$\begingroup\$ @Lundin - I'm not saying that the provided code is the best solution, simply showing how a fairly minimal change to the existing code can set OP on a path to something much more maintainable. \$\endgroup\$ – bta Oct 15 at 20:52
  • 2
    \$\begingroup\$ Small nitpick: OPs code goes to END if getdir(); returns 0. That‘s a hidden state transition from every room to END wich is missing from your translation. \$\endgroup\$ – AlexR Oct 16 at 17:23
20
\$\begingroup\$

The goto debate is ancient, from the year 1966 when Edgar Dijkstra came up with a famous paper called "Go To Statement Considered Harmful". This was controversial and the debate is still going on to this day. Still, most of his conclusions are also valid to this day and most uses of goto is considered harmful spaghetti programming.

However, there's a broad consensus that some uses of goto are acceptable. Specifically:

  • goto should only be used to jump downwards, never upwards.
  • goto should only be used for the sake of error handling and centralized clean-up at the end of a function.

This is an old "design pattern" that I believe/fear originates from BASIC which had "on error goto..." as it's preferred way of error handling. Basically it's only considered OK to use goto like in this made-up example:

status_t func (void)
{
  status_t status = OK;

  stuff_t* stuff = allocate_stuff();
  ...

  while(something)
  {
    while(something_else)
    {
      status = get_status();
      if(status == ERROR)
      {
        goto error_handler; // stop execution and break out of nested loops/statements
      }
    }
  }
 
 goto ok;

 error_handler:
   handle_error(status);

 ok:
   cleanup(stuff);
   return status;
}

Use of goto as in the above example is considered acceptable. There are two obvious benefits: clean way of breaking out of nested statements and centralized error handling and clean-up at the end of the function, avoiding code repetition.

Still it is possible to write the same thing with return and a wrapper function, which I personally find much cleaner and avoids the "goto considered harmful" debate:

static status_t internal_func (stuff_t* stuff)
{
  status_t status = OK;
  
  ...
  
  while(something)
  {
    while(something_else)
    {
      status = get_status();
      if(status == ERROR)
      {
        return status;
      }
    }
  }

  return status;
}

status_t func (void)
{
  status_t status;

  stuff_t* stuff = allocate_stuff();  

  status = internal_func(stuff);

  if(status != OK)
  {
    handle_error(status);
  }

  cleanup(stuff);
  return status;
}

EDIT:

I posted a separate lengthy answer here regarding everything not goto-related. Including a proposal for how to rewrite the whole program using a proper state machine design.

| improve this answer | |
\$\endgroup\$
  • 4
    \$\begingroup\$ @Joshua Since I work mainly with embedded systems, I've seen a whole lot of broken FSMs and there are multiple ways to get them horribly wrong without involving goto. My personal belief is that each state should just return a status to a centralized caller - then a combined decision-maked/error handler reads that status and determines which state to shift to from one single location in the code. Separate state application logic from state transition logic. Otherwise it is very easy to get spaghetti programming. \$\endgroup\$ – Lundin Oct 15 at 6:30
  • 3
    \$\begingroup\$ @Joshua Using function pointers for a state machine is most likely many times slower than using gotos/a switch with loop due to the extra indirection on most CPUs. Depends on the use case whether that's acceptable, but given we're talking C it might be important. \$\endgroup\$ – Voo Oct 15 at 12:05
  • 2
    \$\begingroup\$ Once you start following rules for GOTO you are simply reinventing structured programming. GOTO never went away. It's just typically hidden behind abstractions that enforce these rules for you. Be formal about GOTO and you can avoid spaghetti on your own. \$\endgroup\$ – candied_orange Oct 15 at 14:37
  • 1
    \$\begingroup\$ "Use of goto as in the above example is considered acceptable" - citation needed. Considered by whom? Personally I'd consider it unacceptable and would refuse to pass it if I saw it in a peer review. The goto could trivially be avoided with "&& status != ERROR" in the while conditions. It offers no additional advantages and ends up with slightly weird convoluted flow through the function... as "hey I've found a case where goto is necessary!" always does. \$\endgroup\$ – BittermanAndy Oct 16 at 16:57
  • 1
    \$\begingroup\$ Weird take. "How do I make this code, that I have measured and found to be performing insufficiently well, perform better" would be a valid question. "How do I use this specific construct there is no need to use, to achieve something that it is not capable of achieving", I mean, OK, if that's what you want to be spending your time thinking about, I guess it's your time to waste if you want. In any case I was addressing the claim that the example in this answer is "considered acceptable" which to me is tenuous, and the use of goto here achieves nothing that could not be achieved without it. \$\endgroup\$ – BittermanAndy Oct 16 at 22:53
10
\$\begingroup\$

I'm looking at the goto usage in this program, and I'm going to be controversal here and say it's not that bad. I've seen much worse. Here is a list of things this program does not do.

  • This program never uses goto to go around a variable initialization except where the value is clearly thrown out.
  • This program never does a goto into an inner block. All operational goto statements go to the outermost block.
  • This program does not in fact have haphazard use of goto. All the goto operations center around one operation. (Looks like two, but the strings indicate it is in fact one.)

I've seen the recommendation to build a loop-switch for the state machine. I've done that before. I'm tired of debugging it. (Oh look there's even an answer.) It's harder to do state machines in loop-switch than it is to do them in goto statements.

But looking at the code it is clearly not a tight loop. Nobody cares about microseconds of performance around the loop. We can do better these days. To get back to the ease of goto while still maintaining the scope niceties we can do function state machines. This is higher up the learning ladder, and not everybody gets here early.

It looks as follows: The casts are necessary as the type simply can't be expressed in the type system.

typedef void (*funcptr)(void);
typedef funcptr (*ptrfuncptr)();

int main(int argc, char *argv[])
{
        ptrfuncptr state = START;
        while (state)
            state = (pfuncptr)state();
}

funcptr START()
{
    printf("THE EVIL GOTO DUNGEON\n");
    printf("---------------------\n");
    printf("\nPress a direction key \"W, A, S, D\" followed with 'ENTER' for moving.\n\n");
    return (funcptr)ROOM1;
}

funcptr ROOM1()
{
    giveline();
    printf("Somehow you've managed to wake up at this place. You see a LABEL on the wall.\n");
    printf("\"Do you know what's more evil than an EVIL GOTO DUNGEON?\"\n");
    printf("You're wondering what this cryptic message means.\n");
    where(1, DIR_SE);
    do{
        dir = getdir();
        if(dir == 'S') return (funcptr)ROOM4;
        if(dir == 'E') return (funcptr)ROOM2;
    }while(dir);
    return NULL;
}
// ...

You should not call these ROOM1, ROOM2, ROOM3, etc. If you did that, an array would have suited you better. You should give these all descriptive names. Furthermore, you should change where to take a const char * rather than an int as its first argument.

If you want to win the goto debate, use a lexer as an example. There's just not much else anymore where it's the best way anymore. https://gist.github.com/tomtheisen/cf6afee1210b9625181bb3c23ea0f204#file-csvreader-cs

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ It’s technically illegal to use NULL as a function pointer. More importantly, it’s not type-safe. Since the function pointer gets called immediately, you might be better off implementing it as a tail-recursive call, also eliminating the need for a type cast. Thanks to tail-recursion elimination, this will compile to a jump instruction—just like a goto. Then the function could actually return a value when the game is over. \$\endgroup\$ – Davislor Oct 15 at 4:17
  • 2
    \$\begingroup\$ This is not how you implement a state machine using function pointers. The fact that you are using function pointer casts means that something is fundamentally wrong and dangerous in your code. You do like this except you should also centralize state change decision making for more complex FSMs \$\endgroup\$ – Lundin Oct 15 at 6:41
  • 1
    \$\begingroup\$ @paladin I posted another answer with an example of how to use function pointers to create a finite state machine. Check it out - it is very different from the original code. \$\endgroup\$ – Lundin Oct 15 at 14:08
  • 2
    \$\begingroup\$ @Davislor "It’s technically illegal to use NULL as a function pointer." That's plain wrong. NULL is a null pointer constant and C17 6.3.2.3/3 makes the following guarantee, emphasis mine: "An integer constant expression with the value 0, or such an expression cast to type void *, is called a null pointer constant. If a null pointer constant is converted to a pointer type, the resulting pointer, called a null pointer, is guaranteed to compare unequal to a pointer to any object or function." \$\endgroup\$ – Lundin Oct 15 at 14:26
  • 1
    \$\begingroup\$ @Lundin: The whole point is to get rid of the enum. If you can write down the signature for a function that returns itself, I'll get rid of the cast. \$\endgroup\$ – Joshua Oct 15 at 15:56
9
\$\begingroup\$

Lack of Error Checking on User Input

The function getdir() should check for valid input, perhaps is should receive an array of valid directions. When an invalid direction is entered there should be a message to the user that the input was invalid.

DRY Code

The use of goto is forcing you to repeat code that shouldn't be repeated such as

    where(2, DIR_W);
    do {
        dir = getdir();
        if (dir == 'W') goto ROOM1;
    } while (dir);
    goto END;

Spaghetti Code

The entire program seems to be an example of how to write Spaghetti Code which is unstructured code that is very difficult to write, debug and maintain.

The code would actually be smaller and more understandable if it was structured and used while loops or for loops.

Using Binary in ENUM

The more characters you type the easier it is to make a mistake. Since Bits are important in the enum I would suggest using Octal or Hexadecimal, preferably Hexadecimal. Each enum could then be defined using one character.

Rather than using magic numbers in the code, define masks.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for your reply. My function getdir() does check for valid input and returns -1 at invalid input. What you call as "DRY Code" I see as some kind of "object", isn't it? I agree to your term "Spaghetti Code", but I should've mentioned that I've drawn a "room map" before programming with pen and paper. With this map, the code is very easy to understand. I use my goto only for jumping between the rooms/states. PS I see my binary enum as some kind of jump table, using bitmasks would reduce program speed. But I agree about making mistakes with these kinds of enums. \$\endgroup\$ – paladin Oct 14 at 16:44
  • \$\begingroup\$ DRY Code refers to Don't Repeat Yourself. Dry code can be implemented either using functions or loops depending on the code being used. In this case a loop would probably be good enough. \$\endgroup\$ – pacmaninbw Oct 14 at 23:18
  • 3
    \$\begingroup\$ Regarding binary, it is also not standard C and should be avoided for that reason alone. \$\endgroup\$ – Lundin Oct 15 at 7:05
8
\$\begingroup\$

Quuxplusone's answer briefly mentioned making this data-driven, and I've fleshed out the idea here.

The key realization is that each room varies based only on a few pieces of information: a numeric label, a description string, a set of valid directions, and the label of the room that each valid direction leads to. The original implementation deals with this information using similar code repeated in every 'block'. Implemented that way, making a uniform change requires many modifications in many places, which is error-prone.

Since all of the per-room behavior only depends on a few pieces of information, there is no need to repeat slightly modified code (whether in labeled blocks referenced by goto, cases in a switch statement, or functions referenced using function pointers), which leads to a better realization of the Don't Repeat Yourself principle. Instead of repeated code, you can have a relatively small amount of unique code that acts as required based on external data.

To implement a data-driven approach, some kind of data structure can be used to store the relevant details about each room. Since a room may theoretically have up to 4 neighbors, an easy way to store the neighbors would be as an array of 4 room ids. If we define a constant to represent an invalid room id, the room id stored in each entry directly indicates whether that direction is valid, so there is no need to store the list of valid directions separately.

Since there is a data structure representing a room, we can pass the current room's structure to the various functions that need information about it (where in the original code, and an improved version of getdir that includes looping on invalid inputs) instead of passing the various values separately. This allows some of the benefits of encapsulation, in that future versions of the function can use different fields of the room information structure without requiring every invocation to change.

#include <stddef.h>
#include <limits.h>
#include <stdint.h>
#include <inttypes.h>
#include <stdio.h>
#include <assert.h>

typedef uint_least32_t ROOM_ID;
#define PRI_ROOM_ID PRIuLEAST32
#define INVALID_ROOM_ID UINT_LEAST32_MAX

typedef enum {
    NORTH = 0, //The first 4 values are used as indices, so the exact value matters
    EAST = 1,
    WEST = 2,
    SOUTH = 3,
    ESCAPE_DIRECTION //This is not used as an index, so its value doesn't matter
} DIRECTION;

typedef struct {
    ROOM_ID id;
    const char *description;
    ROOM_ID target_ids[4];
} ROOM;

const ROOM all_rooms[] = {
    {1, "Somehow you've managed to wake up at this place. [...]", {INVALID_ROOM_ID, 2, INVALID_ROOM_ID, 4}},
    //...
};

const ROOM *find_room(ROOM_ID room_id)
{
    size_t i;
    for(i = 0; i < sizeof(all_rooms)/sizeof(all_rooms[0]); ++i)
    {
        if(all_rooms[i].id == room_id)
        {
            return &all_rooms[i];
        }
    }
    return NULL;
}

//Precondition: room is not NULL
void display_where(const ROOM *room)
{
    const struct {
        DIRECTION dir;
        const char *str;
    } descriptions[4] = {{NORTH, "NORTH: W"}, {SOUTH, "SOUTH: S"}, {WEST, "WEST: A"}, {EAST, "EAST: D"}};
    size_t i;
    assert(room != NULL);
    printf("\nYou are in room %" PRI_ROOM_ID ". Where do you want GOTO?\n", room->id);
    for(i = 0; i < 4; ++i)
    {
        if(room->target_ids[descriptions[i].dir] != INVALID_ROOM_ID)
        {
            puts(descriptions[i].str);
        }
        else
        {
            puts(".");
        }
    }
}

//Precondition: room is not NULL
DIRECTION get_dir(const ROOM *room)
{
    while(1)
    {
        int c = getchar();
        switch(c){
            case 'w' :
            case 'W' :
                if(room->target_ids[NORTH] != INVALID_ROOM_ID)
                {
                    return NORTH;
                }
                break;
            case 's' :
            case 'S' :
                if(room->target_ids[SOUTH] != INVALID_ROOM_ID)
                {
                    return SOUTH;
                }
                break;
            //...
            case '\e' :
                return ESCAPE_DIRECTION;
        }
    }
}

int main(void)
{
    const ROOM_ID FIRST_ROOM_ID = 1;
    const ROOM *room = NULL;
    printf("THE EVIL GOTO DUNGEON\n");
    printf("---------------------\n");
    printf("\nPress a direction key \"W, A, S, D\" followed with 'ENTER' for moving.\n\n");

    room = find_room(FIRST_ROOM_ID);
    while(room)
    {
        DIRECTION dir;
        puts("--------------------------------------------------------------------------------");
        puts(room->description);
        display_where(room);
        dir = get_dir(room);
        if(dir == ESCAPE_DIRECTION)
        {
            break;
        }
        else
        {
            room = find_room(room->target_ids[dir]);
        }
    }
}

I'm sure many improvements upon the above are possible, but I think this sufficiently demonstrates the basic idea of data-driven code.


Comments on aspects of the original code besides code repetition:

Defining every combination of values in a bitmask is unnecessary, since a primary benefit of using bitflags in the first place is that they can be manipulated using bitwise operators. For example, in room 6, instead of using where(6, DIR_NSW) you could use where(6, DIR_N | DIR_S | DIR_W). Following this practice overall would mean you can get rid of 11 constant definitions and you wouldn't need to remember which order the flags are listed in the combination values.

Related to using bitfields, your code would be more clear if you used your constants instead of magic numbers. For example, in your where function you could use if(dir & DIR_N) instead of if(dir & 8). To get in a habit that is more generally applicable (even when flags are multi-bit values), you may want to standardize on something like if((dir & DIR_N) == DIR_N).

If you're going to keep your code structure, you could improve it by making getdir accept a description of valid directions as a parameter and have it internally loop until a valid direction is selected. With that change, you could remove all the do/while loops surrounding each invocation (but not the loop body - you still want to actually act on the input).

You could also somewhat reduce the code repetition by creating a function that handles displaying the room and getting the next direction. A signature like DIRECTIONS handle_room(int id, const char *description, DIRECTIONS dirs) might work. Combined with the previous suggestions, each room's code could be made much much shorter, with only the branching repeated.

    ROOM6:
    dir = handle_room(6, "This place doesn't look very promising.\n", DIR_N | DIR_S | DIR_W);
    if(dir == 'N') goto ROOM3;
    if(dir == 'S') goto ROOM9;
    if(dir == 'W') goto ROOM5;
    goto END;
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for your reply. I'm answering to your "Comments on aspects of the original code besides code repetition:": I know about the fact of bit-mask operations, but don't do they increase runtime? Using DIR_N | DIR_S | DIR_W may need more CPU-cycles than just passing a constant. I agree to your statement of not using magic numbers, but I didn't wanted to change the already posted source. I saw that argument of getdir() checking for valid input by itself, but in perspective to my code, this would be redundant and unnecessary. I would reduce the code repetition, but at cost of runtime. \$\endgroup\$ – paladin Oct 16 at 21:37
  • 1
    \$\begingroup\$ Modern (for a very loose definition of modern) optimizing compilers will optimize expressions that contain only constants by evaluating the expression during compilating and inserting just the end result in the output. At runtime, DIR_N | DIR_S | DIR_W will be exactly the same as if you wrote 14 instead. Making getdir() loop inside the function might speed up execution, since it would reduce overall code size (which allows more code to fit in the cache). handle_room might imperceptibly slow things down by a few cycles, but it could have saved significant developer time. \$\endgroup\$ – Extrarius Oct 16 at 21:44
6
\$\begingroup\$

My previous review focused solely on the use of goto. Since then, various answers about state machines and code repetition have popped up. So here is another answer regarding everything else except goto & suggestions for how you could rewrite this better.

C Language

  • Binary literals are not standard C and should therefore be avoided. I can see why you added them as self-documenting code to make sense of the bit masking in the where function. But it probably doesn't make much sense to use bit-fields for this program to begin with - plain numbers might be more readable.
  • The \e escape sequence is not standard C and therefore non-portable. Ironically, escape key has no standardized escape sequence.
  • Never use void func (); empty parenthesis. This is obsolete style in C and means "accept any parameter", which makes such functions less type safe. Instead use void func (void);. (C and C++ are different here.)
  • Assinging -1 to a char is non-portable, since the char type has implementation-defined signedness and might be unsigned on some systems. As a rule of thumb, never use char for anything but text and particularly never use it for any form of arithmetic.
  • Unintuitively, getchar returns an int, not a char. Because it may return EOF. Make a habit of always using an int for the result, even though I don't think it matters in this case.

Style/best practices

  • Your enum should be changed to typedef enum { ... } typename; and then use variables of type typename when you refer to the enumeration constants, not some int or char etc.

  • Use consistent coding style with indention and line breaks. It is very hard to read code such as this:

    if(dir & 8) printf("NORTH: W\n");
    else printf(".\n");
    if(dir & 4) printf("SOUTH: S\n");
    else printf(".\n");
    

    Instead, do this (optionally with { }) :

    if(dir & 8) 
        printf("NORTH: W\n");
    else 
        printf(".\n");
    
  • Ideally all C programs exist in multiple files. In which case it is custom to make local functions static.

Program design

  • The "spaghetti" in this program isn't as much the fault of goto as the fault of decentralizing room changes (state machine changes) and placing them all over the code. Switching out goto for a state/room variable doesn't fix this, it's "stateghetti" instead of "spaghetti", just a different flavour of the same thing.

    My recommended practice for state machines is to centralize all decision making to one single place. Preferably together with error handling. The ideal state machine looks like this:

      for(;;)
      {
        status = state_machine[state]();
        state = decision_maker(status);
      }
    

    Here the state application logic has been separated from state transition logic. So we don't have to dig through all individual states to figure out which one that caused a state change to what. Everything is centralized inside the "decision_maker", including optional error handling.

    I'll make an example for how this can be applied to your code at the bottom of this post.

  • As pointed out in other reviews, this program suffers from a lot of code repetition, which is always a bad thing, particularly during maintenance. This can be fixed by placing all repeated code in the caller and only place room-specific things inside the room logic code. If we rewrite this to a proper state machine, we can fix that problem at the same time.

After a complete make-over, I came up with a main looking like this:

int main (void)
{
    printf("THE EVIL GOTO DUNGEON\n");
    printf("---------------------\n");
    printf("\nPress a direction key \"W, A, S, D\" followed with 'ENTER' for moving.\n\n");

    int current_room = 1;

    for(;;)
    {
      giveline();
      ROOM[current_room]();
    
      int next_room;
      do
      {
        next_room = get_next_room(getdir(), current_room);
      } while(next_room == 0);
      current_room = next_room;
    }    
    
    printf("You never saw me.\n");

    giveline();
    printf("The End\n");
    return 0;
}

Unfortunately, this version lacks any means to escape the dungeon, since my compiler didn't support \e. Otherwise the for(;;) should be replaced with while(stuck_in_dungeon).

The key features here is the ROOM function pointer array, which is a state machine. All decision making has been given to a function called get_next_room, which is the only function that knows how rooms are connected, and which rooms you have access to based on the current one. It's essentially just one big table based on switch:

static int get_next_room (direction dir, int current_room)
{
  switch(current_room)
  {
    case 1:
      if(dir == 'S') return 4;
      if(dir == 'E') return 2;
      break;
    case 2:
      if(dir == 'W') return 1;
      break;

    ...

Complete code follows. Only briefly tested, but the key here is to look at the state machine logic instead of spaghetti.

#include <stdio.h>
#include <stdlib.h>

#define ROOMS 9

typedef enum {
    DIR_0,
    DIR_E,
    DIR_W,
    DIR_WE,
    DIR_S,
    DIR_SE,
    DIR_SW,
    DIR_SWE,
    DIR_N,
    DIR_NE,
    DIR_NW,
    DIR_NWE,
    DIR_NS,
    DIR_NSE,
    DIR_NSW,
    DIR_NSWE,
} direction;


typedef void room_func (void);

static void room1 (void);
static void room2 (void);
static void room3 (void);
static void room4 (void);
static void room5 (void);
static void room6 (void);
static void room7 (void);
static void room8 (void);
static void room9 (void);

room_func* const ROOM [ROOMS+1] = 
{
  NULL, // invalid room
  room1,
  room2,
  room3,
  room4,
  room5,
  room6,
  room7,
  room8,
  room9,
};

static int get_next_room (direction dir, int current_room);

static void giveline(void);
static void where(int room, direction dir);
static char getdir (void);

int main (void)
{
    printf("THE EVIL GOTO DUNGEON\n");
    printf("---------------------\n");
    printf("\nPress a direction key \"W, A, S, D\" followed with 'ENTER' for moving.\n\n");

    int current_room = 1;

    for(;;)
    {
      giveline();
      ROOM[current_room]();
    
      int next_room;
      do
      {
        next_room = get_next_room(getdir(), current_room);
      } while(next_room == 0);
      current_room = next_room;
    }    
    
    printf("You never saw me.\n");

    giveline();
    printf("The End\n");
    return 0;
}


static void room1 (void)
{
    printf("Somehow you've managed to wake up at this place. You see a LABEL on the wall.\n");
    printf("\"Do you know what's more evil than an EVIL GOTO DUNGEON?\"\n");
    printf("You're wondering what this cryptic message means.\n");
    where(1, DIR_SE);
}

static void room2 (void)
{
    printf("Besides another LABEL, this room is empty.\n");
    printf("\"Let's play a game!\"\n");
    where(2, DIR_W);
}

static void room3 (void)
{
    printf("Man, dead ends are boring.\n");
    printf("Why can't I escape this nightmare?\n");
    where(3, DIR_S);
}


static void room4 (void)
{
    printf("Is this a real place, or just fantasy?\n");
    printf("\"All good things come in three GOTOs.\"\n");
    where(4, DIR_NSE);
}

static void room5 (void)
{
    printf("This is a big river crossing. I guess I need to JUMP.\n");
    where(5, DIR_SWE);
}

static void room6 (void)
{
    printf("This place doesn't look very promising.\n");
    where(6, DIR_NSW);
}

static void room7 (void)
{
    printf("\"Give a man a LOOP and you feed him FOR a WHILE;\n");
    printf(" teach a man a GOTO and you feed him for a RUNTIME.\"\n");
    where(7, DIR_NE);
}

static void room8 (void)
{
    printf("This looks like an endless LOOP of rooms.\n");
    where(8, DIR_NW);
}

static void room9 (void)
{
    printf("You've found your old friend Domino. He doesn't look scared, like you do.\n");
    printf("\n\"Listen my friend,\n");
    printf(" If you want to escape this place, you need to find the escape sequence.\n");
    printf("\nWhat does this mean? There no standardized escape sequence for the ESCAPE KEY!\n");
    printf("\nAAAAAH!!!\n");
    where(9, DIR_N);
}

static int get_next_room (direction dir, int current_room)
{
  switch(current_room)
  {
    case 1:
      if(dir == 'S') return 4;
      if(dir == 'E') return 2;
      break;
    case 2:
      if(dir == 'W') return 1;
      break;
    case 3:
      if(dir == 'S') return 6;
      break;
    case 4:
      if(dir == 'N') return 1;
      if(dir == 'S') return 7;
      if(dir == 'E') return 5;
      break;
    case 5:
      if(dir == 'S') return 8;
      if(dir == 'W') return 4;
      if(dir == 'E') return 6;
      break;
    case 6:
      if(dir == 'N') return 3;
      if(dir == 'S') return 9;
      if(dir == 'W') return 5;
      break;
    case 7:
      if(dir == 'N') return 4;
      if(dir == 'E') return 8;
      break;
    case 8:
      if(dir == 'N') return 5;
      if(dir == 'W') return 7;
      break;
    case 9:
      if(dir == 'N') return 6;
      break;
  }
  return 0;
}

static void giveline(void){
    printf("--------------------------------------------------------------------------------\n");
}

static void where(int room, direction dir){
    printf("\nYou are in room %i. Where do you want GOTO?\n", room);
    if(dir & 8) printf("NORTH: W\n");
    else printf(".\n");
    if(dir & 4) printf("SOUTH: S\n");
    else printf(".\n");
    if(dir & 2) printf("WEST:  A\n");
    else printf(".\n");
    if(dir & 1) printf("EAST:  D\n");
    else printf(".\n");
}

static char getdir (void){
    char c = getchar();
    switch(c){
        case 'w' :
        case 'W' :
            return 'N';
        case 's' :
        case 'S' :
            return 'S';
        case 'a' :
        case 'A' :
            return 'W';
        case 'd' :
        case 'D' :
            return 'E';
    }
    return -1;
}

Unfortunately this also ruined all the goto puns :(

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your reply. I'm using GCC compiler. I didn't know that using binary literals aren't C-conform, nor \e is, I'll keep that in mind. If it isn't to much trouble for you, may you explain a bit more detailed about the void func();-thing? May I use this "accept any argument" also to my advantage? I knew about the getchar(), but I decided to use it exactly as I did, because I knew it wouldn't matter. Is that bad practice too? I appreciate your work very much, especially the idea with those function pointers. But somehow, I also like my GOTOs, maybe I'm too inexperienced. \$\endgroup\$ – paladin Oct 15 at 15:41
  • \$\begingroup\$ The standard escape sequence for escape is \033 (ASCII won). \$\endgroup\$ – Joshua Oct 15 at 15:59
  • 2
    \$\begingroup\$ @paladin Empty () can be abused for function pointer compatibility tricks, but since it's formally obsolete style and may get removed from the C language, you shouldn't use it. See stackoverflow.com/a/63775659/584518. As for gotos, once you've had the "goto considered harmful" debate some 20-30 times, you'll avoid that keyword just for peace of mind. \$\endgroup\$ – Lundin Oct 16 at 6:52
  • 1
    \$\begingroup\$ @Joshua "But now you can't even have holes in your state enum" I take it you mean holes in the function pointer table? That's good! Why would I want holes in it? \$\endgroup\$ – Lundin Oct 16 at 6:57
  • 1
    \$\begingroup\$ @paladin No, you don't get any form of overloading from empty parameter lists, nor can you exploit them for any other benefit. This is simply an obsolete feature of the language, from a time before C was standardized, when the language behaved very differently. With an empty parameter list in the declaration, each time you called the function, the compiler would deduce the parameters based on the arguments you specified, using the default promotion rules. It was a mess. Functions should always be declared with the parameters they actually accept. If none, (void). \$\endgroup\$ – Cody Gray Oct 17 at 8:27
6
\$\begingroup\$

I suggest reading Donald Knuth's article Structured Programming With goto statements (1974) Originally published in Computing Surveys 6, and the second chapter of his book Literate Programming. There he makes excellent points on both sides of the goto debate.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for this suggestion. I looked into it, it looks solid, but a bit outdated. I know that structured text made the usage of goto nearly unneeded. I also know about the advantages of goto, that's why I'm using it in the first place. What I really would like to see is an up to date recommendation of when and how to use goto. Especially syntax for constructing "custom multi-dimensional and interleaving loops with variable labels", just the total nightmare used in an accurate way. \$\endgroup\$ – paladin Oct 17 at 0:14
5
\$\begingroup\$

I'll take an alternative tack from the other answerers: Your code organization is not bad, and all that remains to do is eliminate redundancy. Notice that every room in your game has the same basic structure:

ROOM7:
giveline();
printf("\"Give a man a LOOP and you feed him FOR a WHILE;\n");
printf(" teach a man a GOTO and you feed him for a RUNTIME.\"\n");
where(7, DIR_NE);
do{
    dir = getdir();
    if(dir == 'N') goto ROOM4;
    if(dir == 'E') goto ROOM8;
}while(dir);
goto END;

If we had a macro something like DEFINE_ROOM, we could just write

DEFINE_ROOM(7, DIR_NE,
    "\"Give a man a LOOP and you feed him FOR a WHILE;\n"
    " teach a man a GOTO and you feed him for a RUNTIME.\"\n",
    'N', ROOM4,
    'E', ROOM8
);

It's pretty complicated to write a C macro that can take an arbitrary number of room exits, so instead, I'll write separate macros for each combination of room directions.

#define DEFINE_ROOM_NE(num, desc, roomN, roomE) \
    ROOM##num: giveline(); printf(desc); \
    where(num, DIR_NE); \
    while (dir = getdir()) { \
        if (dir == 'N') goto roomN; \
        if (dir == 'E') goto roomE; \
    } \
    goto END;
#define DEFINE_ROOM_NW(num, desc, roomN, roomW) \
    ROOM##num: giveline(); printf(desc); \
    where(num, DIR_NW); \
    while (dir = getdir()) { \
        if (dir == 'N') goto roomN; \
        if (dir == 'W') goto roomW; \
    } \
    goto END;

In fact, wait a minute, we can do better! Notice that if you enter a bogus direction, you just go around the loop again. So I can add a SAME##n label to do this:

#define DEFINE_ROOM(dirs, num, desc, roomN, roomS, roomE, roomW) \
    ROOM##num: giveline(); printf(desc); \
    where(num, DIR_##dirs); \
    while (dir = getdir()) { \
        if (dir == 'N') goto roomN; \
        if (dir == 'S') goto roomS; \
        if (dir == 'E') goto roomE; \
        if (dir == 'W') goto roomW; \
        SAME##num: ; \
    } \
    goto END;
#define DEFINE_ROOM_N(n, roomN, d) DEFINE_ROOM(N, n, d, roomN, SAME##n, SAME##n, SAME##n)
#define DEFINE_ROOM_S(n, roomS, d) DEFINE_ROOM(S, n, d, SAME##n, roomS, SAME##n, SAME##n)
#define DEFINE_ROOM_E(n, roomE, d) DEFINE_ROOM(E, n, d, SAME##n, SAME##n, roomE, SAME##n)
#define DEFINE_ROOM_W(n, roomW, d) DEFINE_ROOM(W, n, d, SAME##n, SAME##n, SAME##n, roomW)
#define DEFINE_ROOM_NS(n, roomN, roomS, d) DEFINE_ROOM(NS, n, d, roomN, roomS, SAME##n, SAME##n)
[...]

And now your whole game fits in an appropriate number of lines:

DEFINE_ROOM_SE(1, ROOM4, ROOM2,
    "Somehow you've managed to wake up at this place. You see a LABEL on the wall.\n"
    "\"Do you know what's more evil than an EVIL GOTO DUNGEON?\"\n"
    "You're wondering what this cryptic message means.\n"
);
DEFINE_ROOM_W(2, ROOM1,
    "Besides another LABEL, this room is empty.\n"
);
DEFINE_ROOM_S(3, ROOM6,
    "Man, dead ends are boring.\n"
    "Why can't I escape this nightmare?\n"
);
DEFINE_ROOM_NSE(4, ROOM1, ROOM7, ROOM5,
    "Is this a real place, or just fantasy?\n"
    "\"All good things come in three GOTOs.\"\n"
);
DEFINE_ROOM_SWE(5, ROOM8, ROOM4, ROOM6,
    "This is a big river crossing. I guess I need to JUMP.\n"
);
DEFINE_ROOM_NSW(6, ROOM3, ROOM9, ROOM5,
    "This place doesn't look very promising.\n"
);
DEFINE_ROOM_NE(7, ROOM4, ROOM8,
    "\"Give a man a LOOP and you feed him FOR a WHILE;\n");
    " teach a man a GOTO and you feed him for a RUNTIME.\"\n"
);    
DEFINE_ROOM_NW(8, ROOM5, ROOM7,
    "This looks like an endless LOOP of rooms.\n"
);
DEFINE_ROOM_N(9, ROOM6,
    "You've found your old friend Domino. He doesn't looks scared, like you do.\n"
    "\n\"Listen my friend,\n"
    " If you want to escape this place, you need to find the ESCAPE KEY.\"\n"
    "\nWhat does this mean?\n"
);

I see you're intending to add more motion verbs, such as JUMP. Do you see how to fit JUMP into this pattern? How many motion verbs can you support before this one-macro-per-kind-of-room pattern starts to break down?

The next step is to stop writing macros that directly generate code and start writing macros that generate data tables, which are then passed to code that knows how to loop over the table's entries and do something for each entry (such as test, "Did the user enter the motion verb in column 1? if so, goto the room in column 2").

For a (classic!) worked example, see Donald Knuth's CWEB port of Adventure, or my own C99 port here.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ So what happens when we try to add ROOM3A? \$\endgroup\$ – Joshua Oct 15 at 2:43
  • \$\begingroup\$ @Joshua: Well, void where(int room, unsigned char dir) isn't really set up to handle non-numeric room numbers. But if you wanted to have a 3A, you could change that function's signature to where(const char *room, unsigned char dir) and then insert a # character in where(#num, DIR_##dirs). \$\endgroup\$ – Quuxplusone Oct 15 at 3:01
  • 3
    \$\begingroup\$ Code repetition is bad practice and should be avoided. But turning your program into a secret macro language hell is extremely bad practice! This is like putting out a small kitchen fire by dropping a bomb on the building. If I'm a C programmer tasked to maintain a C program, I already know the C language. I should expect to find that one, not some secret macro language that I now have to study and learn. \$\endgroup\$ – Lundin Oct 15 at 6:47
  • \$\begingroup\$ @Lundin By that logic, I should never use any libraries aside from the standard library. I already know the C language; I shouldn't have to study and learn a "secret function language". The macros halve the code size, make the logic very clear, and the definitions are right there with the rest of the code. And you definitely can read them, because you know the C language and macros are a part of that. (That said, it would probably be better to have a single DEFINE_ROOM macro, with a NONE value that you can pass in for directions that aren't available, rather than having 15 similar macros.) \$\endgroup\$ – Ray Oct 15 at 21:24
  • \$\begingroup\$ @Ray I guess you haven't maintained many "macro jungles" like this then. It's a nightmare. Errors are subtle, compiler warnings point at strange lines, no type safety, horrible to debug and so on. The original goto spaghetti with code repetition is still better than this alternative. \$\endgroup\$ – Lundin Oct 19 at 6:29
4
\$\begingroup\$

The program you give as an example would be better-designed as a finite state machine. The states could be implemented as mutually-recursive tail calls, which a modern compiler would optimize to jump instructions, just like a goto. You could then represent the states as function pointers and eliminate the switch.

Joshua has a demonstration of code somewhat like that, and Lundin refines it, but it might be better to have each room call the next room function mutually tail-recursively, until the game is over and the last room function finally returns a value. For example, it might return the score.

An optimizing compiler will compile the tail call, since it is to a function with the same type, as an unconditional jump. That is also how it would compile a goto. Since you are no longer discombobulating the C parser by making a function return a pointer to a function of the same type, you no longer need to cast function pointers to different types. You also no longer need to have invalid null function pointers in your program that would crash it if they are ever called. Doing it this way would preserve type safety and let the compiler ensure that you are actually calling valid code.

The next step might be to observe that most or all of these rooms perform the same operations on different data, write this code as a single function that takes a data structure, and pass along the room data as a parameter of the function. This could be a pointer to static data, or an index into an array of rooms. Either is a constant-time look-up, simpler and more efficient than a switch block.

If you do need more than one type of room with different code, and you pass around both room data and a function pointer to the room code, you have reinvented polymorphic objects with data members and methods.

I’ve used a goto once myself, not counting programs I wrote in BASIC as a child. It was to break out of multiple levels of a nested loop. I considered, but rejected, refactoring those levels of the loop as a function I could return from, or creating a loop-control variable. I don’t regret it.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Good for finding finite state machine. However you miss goto is directly writing a finite state machine. I've seen truly terrible goto abuse but this isn't it. \$\endgroup\$ – Joshua Oct 15 at 2:23
  • \$\begingroup\$ @Joshua I realize that the goto labels could be considered states in a FSM, yes. No argument there. In the spirit of a code review: it’s no more efficient to do it this way, and much less maintainable. \$\endgroup\$ – Davislor Oct 15 at 2:30
  • 2
    \$\begingroup\$ My experience is the reverse. The switch is harder to maintain. \$\endgroup\$ – Joshua Oct 15 at 2:42
  • \$\begingroup\$ @Joshua I suggested a way to eliminate the switch, so that works out nicely! \$\endgroup\$ – Davislor Oct 15 at 2:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.