Is this a clear use of goto?

Question

Hi All,

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.

Answer 1

Personally, I would have this in a separate method that returns a status code of success or failure. Then, in the code that would call this method, I can have some magic number of times that I would keep trying this until the status code is "Success". I just don't like using try/catch for control flow.

Answer 2

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

Answer 3

No, it's not okay: http://xkcd.com/292/

Answer 4

maybe im missing something but why cant you just use a while loop? this will give you the same loop forever if you have an exception (which is bad code) functionality that your code gives.

IDatabase database = null;

while(database == null){
   try
   {
        database = databaseLoader.LoadDatabase();
   }
   catch(Exception 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);
        //just in case??
        database = null;
   }
 }

if you have to use goto in your normal code, you're missing logical flow. which you can get using standard constructs, if, while, for etc..

Answer 5

Is it clear? Not really. What you actually want to do, I think, is first try to load the database and then, if that didn't work, try to load it a different way. Is that right? Let's write the code that way.

IDatabase loadedDatabase = null;

// first try
try
{
    loadedDatabase = databaseLoader.LoadDatabase();
}
catch(Exception e) { }  // THIS IS BAD DON'T DO THIS

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

This more clearly illustrates what you're actually doing. As an added bonus, other programmers won't gouge out your eyes. :)

NOTE: you almost certainly don't want to catch Exception. There's likely a more specific exception that you would rather be catching. This would also catch TheComputerIsOnFireException, after which it isn't really worth retrying.

Answer 6

On a side note, I think there is potential for an endless loop if you always get an exception.

Technically there is nothing wrong with your goto structure, but for me, I would opt for using a while loop instead. Something like:

IDatabase database = null;

bool bSuccess = false;
int iTries = 0
while (!bSuccess) // or while (database == null)
{
    try
    {
        iTries++;
        database = databaseLoader.LoadDatabase();
        bSuccess = true;
    }
    catch(DatabaseLoaderException e)
    {
        //Avoid an endless loop
        if (iTries > 10)
             throw 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);
    }
}