Is this explicit use of goto?

Question

Just wondering if using goto in C # is considered understandable:

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 this is normal because the fragment is small and should make sense. Is there any other way when people usually recover from such errors when you want to repeat the operation after handling the exception?

Edit: That was fast. To answer a few questions and clarify the situation a bit is part of a process that is essentially being transformed from another project. The _userInteractor.GetDatabaseConnector () call is the part that will determine if the user wants to try again (possibly with a different database than the one in which they are loaded). If it returns null, then a new database connection was not specified, and the operation completed completely.

I have no idea why I did not think to use a while loop. It should be too close by 5pm.

Edit 2: I looked at the LoadDatabase () method and it will throw a DatabaseLoaderException if it doesn't work. I updated the code above to catch this exception instead of Exception.

Edit 3:. By all accounts,

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

Answer 1

Is there any other way for people to recover from such errors when you want to repeat the operation after handling the exception?

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

UPDATE:

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

"I have no idea what happened, but all I did is explode ... so let's do it again."

Eliminate certain errors from which you can recover, and let the remaining bubbles go to the next level. Any exceptions that do this to the very top are true errors at this point.

UPDATE 2:

So, instead of continuing a rather lengthy discussion with the help of comments, which I will describe in the example with a semi-pseudo-code.

The general idea is that you just need to restructure the code for testing and improve the user interface a bit.

 //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 certainly not an example of production. I hope you get a general idea. Rebuild the program to avoid unnecessary gaps in the application flow.

Cheers, Josh

Answer 2

maybe something is missing, but why can't you use a while loop? this will give you the same loop forever if you have an exception function (which is bad code) 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 need to use goto in your regular code, you are missing a logical stream. which you can get with standard designs if, while for, etc.

Answer 3

Personally, I would like to have this in a separate method that returns a success or failure status code. Then, in the code that this method will call, I can get some magic numbers that I would continue to try until the status code becomes “Success”. I just don't like using try / catch for a control flow.

Answer 4

It is clear? Not really. What you really want to do, I think, will try to load the database first, and then, if that doesn't work, try loading it differently. It is right? Let's write the code this 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 are actually doing. As an added bonus, other programmers will not look out for you. :)

NOTE. You almost certainly don't want to break an Exception. This is probably the more specific exception that you prefer to catch. This would also catch TheComputerIsOnFireException, after which you should not try again.

Answer 5

No, this is not normal: http://xkcd.com/292/

Answer 6

On the other hand, I think there is a chance of an infinite loop if you always get an exception.

There is technically nothing wrong with your goto structure, but for me, I would prefer to use a while loop. 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); } } 

Answer 7