How bad is this goto?

Question

I created a tetris game where you can restart after a game over. I implemented this quick and dirty with a goto (see code). The Game class relies on destructors, are these called with these goto's? How bad is this goto, is it acceptable, or what should I do instead?

int APIENTRY WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow) {
    // initiate sdl
    sdl_init();

    // seed rng
    srand(time(NULL));

    newgame: // new game label
    Game game(GAME_WIDTH, GAME_HEIGHT, 1, screen);

    // keydowns
    bool fastfall = false;
    bool gamerunning = true;
    Uint32 lastupdate = 0;

    while (gamerunning && game.isalive()) {
        // game running stuff here
    }

    // game over stuff here

    while (gamerunning) {
        if (SDL_PollEvent(&event)) {
            if (event.type == SDL_QUIT) {
                gamerunning = false;
            } else if (event.type == SDL_KEYDOWN) {
                if (event.key.keysym.sym == SDLK_r) goto newgame; // yay a new game!
            }
        }
    }

    TTF_Quit();
    SDL_Quit();
    return 0;
}

Answer 1

You could easily avoid this by putting the majority of this function in a while loop, and setting a flag to break out of it.

In C, the only real "acceptable" use of goto was for jumping to common clean-up code in the case of errors. In C++, you can avoid even this with exceptions. So really, there's no excuse!

Answer 2

To answer the question about destructors no one else seems to have covered. According to 6.6/2 the destructors will be called for you. Quote:

On exit from a scope (however accomplished), destructors (12.4) are called for all constructed objects with automatic storage duration (3.7.2) (named objects or temporaries) that are declared in that scope, in the reverse order of their declaration. Transfer out of a loop, out of a block, or back past an initialized variable with automatic storage duration involves the destruction of variables with automatic storage duration that are in scope at the point transferred from but not at the point transferred to.

However I still don't suggest goto in this case at all. It doesn't clearly (to me anyway) indicate what's happening. You should just use a while loop and have it operate on the conditions instead.

Even something as simple as this should be more clear (although there's probably a way to rewrite it without the inner break). It's perfectly obvious that the locals are cleaned up used inside a while loop like this:

int APIENTRY WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow) {
    // initiate sdl
    sdl_init();

    // seed rng
    srand(time(NULL));

    bool gamerunning = true;
    while(gamerunning)
    {
        Game game(GAME_WIDTH, GAME_HEIGHT, 1, screen);

        // keydowns
        bool fastfall = false;
        Uint32 lastupdate = 0;

        while (gamerunning && game.isalive()) {
            // game running stuff here
        }

        // game over stuff here

        while (gamerunning) {
            if (SDL_PollEvent(&event)) {
                if (event.type == SDL_QUIT) {
                    gamerunning = false;
                } else if (event.type == SDL_KEYDOWN) {
                    if (event.key.keysym.sym == SDLK_r) break; // yay a new game - get out of the "what to do next" loop.
                }
            }
        }
    }

    TTF_Quit();
    SDL_Quit();
    return 0;
}

Answer 3

Instead of a goto you can put everything from your newgame tag to the end of the while loop in a function. This function's return value would tell you if you have to run again. So it would be something like:

...
srand(time(NULL));

while (runGame())
{
}

TTF_Quit();
...

You would have to pass runGame() any parameters from your main function that you use in your game code and return a 1 where the code uses the goto and a zero when it is the last game.

Answer 4

Break the significant blocks up into functions and then rather than calling goto, just call a function instead.

Answer 5

Gotos are rarely good to use. The exception seems to be for cleanup, where you need to quickly break out of many nested loops, free up some memory and exit. This here can easily be replaced with a while loop. If left as is it will only make debugging and maintenance harder.

Answer 6

There are some good times to use goto (eg: Implementing a state machine), but I'm not sure this is really one of them.

If it were me, I'd put the "game" code in a subroutine, exit out of it when done, and then let the higher-level routine chose to start a new game or something.

Answer 7

int APIENTRY WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow) {
    // initiate sdl
    sdl_init();

    // seed rng
    srand(time(NULL));


    while (1) { 
    Game game(GAME_WIDTH, GAME_HEIGHT, 1, screen);

    // keydowns
    bool fastfall = false;
    bool gamerunning = true;
    Uint32 lastupdate = 0;

    while (gamerunning && game.isalive()) {
        // game running stuff here
    }

    // game over stuff here
    restart_game = false; 
    while (gamerunning) {
        if (SDL_PollEvent(&event)) {
            if (event.type == SDL_QUIT) {
                gamerunning = false;
            } else if (event.type == SDL_KEYDOWN) {
                if (event.key.keysym.sym == SDLK_r) { 
                      restart_game = true; break; 
                } 
            }
        }
    }
    if (!restart_game) break; 
    }

    TTF_Quit();
    SDL_Quit();
    return 0;
}