Is it ok to use goto in catch?

Question

start:
try
{
    oleDbDataAdapter1.Fill(myDataTable);
}
catch (Exception ex) {
    DialogResult res = MessageBox.Show("Can't load data:\n" + ex.Message, "Error",
        MessageBoxButtons.RetryCancel, MessageBoxIcon.Error);
    if (res == DialogResult.Retry)
    {
        goto start;
    }
}

1. Is it ok to use goto operators, which lead outside a try/catch block? What consequences may it lead to?
2. How can I make this code better?

Answer 1

I'm personally not a fan of goto's and to be honest I don't even know if I've ever used on in c#. I tend to find they are a bit hard to follow as you have to jump around a bit too much and the natural flow of the code is potentially broken.

Here's an idea for using a different approach (Updated as suggested by svick)

string errorMessage;
while(!TryTableFill(myDataTable, out errorMessage))
{
    DialogResult result = MessageBox.Show("Can't load data:\n" + errorMessage, "Error", MessageBoxButtons.RetryCancel, MessageBoxIcon.Error);

    if(result != DialogResult.Retry)
        break;
}

private boolean TryTableFill(myDataTable, out string errorMessage)
{
    errorMessage = string.Empty;

    try
    {
       oleDbDataAdapter1.Fill(myDataTable);
       return true;   
    }
    catch (Exception ex) {  
       errorMessage = ex.Message;
       return false;
    }       
}

or alternatively (I like this way but I'm not sure if it's better. Added just to provide an alternative):

private boolean TryTableFill(myDataTable, out string errorMessage)
{
    errorMessage = string.Empty;

    try
    {
       oleDbDataAdapter1.Fill(myDataTable);
    }
    catch (Exception ex) {  
       errorMessage = string.Format("Error occured: {0}", ex.Message);
    }       

    return !string.isNullOrEmpty(errorMessage);
}

Answer 2

You can also use recursion to avoid the goto in this case. Wrap the code in a method and let the method call itself instead of using goto.

Warning: Recursion can lead to a stack overflow. It's no problem here as long as the user doesn't click "Retry" too often, but when you remove the dialog the code becomes dangerous. Using an iterative solution (including the goto solution) doesn't have that drawback.

private void TableFill(myDataTable)
{
    try
    {
        oleDbDataAdapter1.Fill(myDataTable);
    }
    catch (Exception ex) {
        DialogResult res = MessageBox.Show("Can't load data:\n" + ex.Message, "Error",
            MessageBoxButtons.RetryCancel, MessageBoxIcon.Error);
        if (res == DialogResult.Retry)
        {
            TableFill(myDataTable);
        }
    }
}

Answer 3

Do not use goto as it is a clear sign of what is called spaghetti code. @dreza has provided a much better solution. You routines should be tightly cohesive, meaning they do one thing and they do it well. Only wrap calls in try/catch if the call might throw an exception, then handle the exception and continue execution.

Answer 4

You probably want to use some form of asynchronous callback to call the method after its ended. It has been a year or so since I've programmed in C# and I've probably messed up the syntax, but you can use the Dispatcher to do this. You may need to use the Dispatcher appropriate to your UI framework (wpf, winforms, etc).

void myMethodName(myDataTable) 
{
    try { oleDbDataAdapter1.Fill(myDataTable); }
    catch (Exception ex) 
    {
        DialogResult res = MessageBox.Show("Can't load data:\n" + ex.Message, "Error", MessageBoxButtons.RetryCancel, MessageBoxIcon.Error);
        if (res == DialogResult.Retry) 
            Dispatcher.BeginInvoke(() => { myMethodName(myDataTable); }); // myMethodName will be called after the method ends.
     }
}

This uses the dispatcher to schedule a myMethodName() to be called again after your current instance of myMethodName() has exited and other queued methods have executed. This should avoid any problems of a stackoverflow.

Update: Another way to do this, if you want to stay in the same method could be something like this. It is fairly concise and give the closest match to your current architecture. It does block which may or may not be desirable.

do 
{        
    DialogResult retry = null;
    try { oleDbDataAdapter1.Fill(myDataTable); }
    catch (Exception ex) { retry = MessageBox.Show("Can't load data:\n" + ex.Message, "Error", MessageBoxButtons.RetryCancel, MessageBoxIcon.Error); }  

} while (retry == DialogResult.Retry);

Answer 5

I'm not a fan of the TrySomething with out parameter pattern, so I would sooner do the following:

while (true) {
    try {
        possiblyFailingOperation();
        break;
    }
    catch (Exception e) {
        reportError();
        if (abortRequested())
            throw;
    }
}

A TrySomething function makes sense when there's several places that call the same exception-throwing function and they all want to immediately catch and handle a single kind of exception said function may throw. For example, TryParse often makes sense as parsing may be common and may have only one failure state (no parse). It makes sense to do

if (!TryParse(x, out i))
    i = default_value;

When there's only one function that calls the TrySomething function, the added benefit is significantly less; we end up obscuring what exception is thrown and losing the ability to rethrow if necessary.

If this is all insignificant, a try function may be worthwhile; however, I'd use it in reaction to a common pattern, not in anticipation of one.