Search code examples
c#goto

Is this a clear use of goto?


Just wondering if this is considered a clear use of goto in C#:

IDatabase database = null;

LoadDatabase:
try
{
    database = databaseLoader.LoadDatabase();
}
catch(DatabaseLoaderException e)
{
    var connector = _userInteractor.GetDatabaseConnector();
    if(connector == null)
        throw new ConfigException("Could not load the database specified in your config file.");
    databaseLoader = DatabaseLoaderFacade.GetDatabaseLoader(connector);
    goto LoadDatabase;
}

I feel like this is ok, because the snippet is small and should make sense. Is there another way people usually recover from errors like this when you want to retry the operation after handling the exception?

Edit: That was fast. To answer a few questions and clarify things a bit - this is part of a process which is essentially converting from a different kind of project. The _userInteractor.GetDatabaseConnector() call is the part which will determine if the user wants to retry (possibly with a different database than the one in the config they are loading from). If it returns null, then no new database connection was specified and the operation should fail completely.

I have no idea why I didn't think of using a while loop. It must be getting too close to 5pm.

Edit 2: I had a look at the LoadDatabase() method, and it will throw a DatabaseLoaderException if it fails. I've updated the code above to catch that exception instead of Exception.

Edit 3: The general consensus seems to be that

  • Using goto here is not necessary - a while loop will do just fine.
  • Using exceptions like this is not a good idea - I'm not sure what to replace it with though.

Solution

  • Is there another way people usually recover from errors like this when you want to retry the operation after handling the exception?

    Yes, in the calling code. Let the caller of this method decide if they need to retry the logic or not.

    UPDATE:

    To clarify, you should only catch exceptions if you can actually handle them. Your code basically says:

    "I have no idea what happened, but whatever I did caused everything to blow up... so lets do it again."

    Catch specific errors that you can recover from, and let the rest bubble up to the next layer to be handled. Any exceptions that make it all the way to the top represent true bugs at that point.

    UPDATE 2:

    Ok, so rather than continue a rather lengthy discussion via the comments I will elaborate with a semi-pseudo code example.

    The general idea is that you just need to restructure the code in order to perform tests, and handle the user experience a little better.

    //The main thread might look something like this
    
    try{
        var database = LoadDatabaseFromUserInput();
    
        //Do other stuff with database
    }
    catch(Exception ex){
        //Since this is probably the highest layer,
        // then we have no clue what just happened
        Logger.Critical(ex);
        DisplayTheIHaveNoIdeaWhatJustHappenedAndAmGoingToCrashNowMessageToTheUser(ex);
    }
    
    //And here is the implementation
    
    public IDatabase LoadDatabaseFromUserInput(){
    
        IDatabase database = null;
        userHasGivenUpAndQuit = false;
    
        //Do looping close to the control (in this case the user)
        do{
            try{
                //Wait for user input
                GetUserInput();
    
                //Check user input for validity
                CheckConfigFile();
                CheckDatabaseConnection();
    
                //This line shouldn't fail, but if it does we are
                // going to let it bubble up to the next layer because
                // we don't know what just happened
                database = LoadDatabaseFromSettings();
            }
            catch(ConfigFileException ex){
                Logger.Warning(ex);
                DisplayUserFriendlyMessage(ex);
            }
            catch(CouldNotConnectToDatabaseException ex){
                Logger.Warning(ex);
                DisplayUserFriendlyMessage(ex);
            }
            finally{
                //Clean up any resources here
            }
        }while(database != null); 
    }
    

    Now obviously I have no idea what your application is trying to do, and this is most certainly not a production example. Hopefully you get the general idea. Restructure the program so you can avoid any unnecessary breaks in application flow.

    Cheers, Josh