Is using "goto" acceptable in this situation?

Question

The following is pseudocode:

myGoto:
try
{
   // do some db updating
   myDB.doOptimisticConcurrency();

} catch (MyConcExeption ex) {

   if (tried < fiveTimes) {
       myDB.Refresh();
       tried++;
       goto myGoto;
   }

}

I have several try-catch blocks in one method, and I don't want to reinvoke my method from the beginning for every thrown exception. Is using goto acceptable in this situation?

Answer 1

Using goto is almost never acceptable, it leads to spaghetti code and get your code less readable.

In your case a simple loop would make your code more readable.

Answer 2

You could just use a loop.

Answer 3

You can change it to:

while (tried < fiveTimes)
try
{
   // do some db updating
   myDB.doOptimisticConcurrency();
   break;
}
catch (MyConcExeption ex)
{
   tried++;
   myDB.Refresh();
}

Answer 4

You shouldn't be throwing an exception if the situation is not, well, exceptional. Why don't you add a retry argument to that method and throw the exception internally only if the retry count is exceeded?

EDIT: Like others have suggested, a loop is a better alternative as well. However, that looks like your method, not a method wrapped in some library that you can't modify. If I am correct, I would still go with the retry parameter and only throw an exception if all retries fail. If you expect that this method will sometimes fail on the first attempt that should not be an exception.

Answer 5

I wouldn't use "goto" - but you might want to write a little helper method. For example:

public static void TryMultiple<E>(Action action, int times) where E : Exception
{
    E lastException = null;
    for (int i = 0; i < times; i++)
    {
        try
        {
            action();
            return; // Yay, success!
        }
        catch (E exception)
        {
            // Possibly log?
            lastException = exception;
        }
    }
    throw new RetryFailedException("Retry failed " + times + " times",
                                   lastException);
}

That's just a sketch of the solution - you'll want to adapt it accordingly. Anyway, this basically lets you perform retries in the face of a semi-accepted exception in a reusable manner. You'll probably use a lambda expression to express the action, or sometimes just a method group for a single method call:

TryMultiple<MyConcurrencyException>(myDB.doOptimisticConcurrency, 5);

Answer 6

This is much better:

private void MyMethod()
{
   MyMethod(5);
}    

private void MyMethod(int triesLeft)
{
   if(triesLeft == 0)
      return;  // or throw

   try
   {
      // do some db updating
      myDB.doOptimisticConcurrency();
   }       
   catch (MyConcExeption ex) 
   {
       myDB.Refresh(); 
       MyMethod(triesLeft - 1);
   }
}