# Is it ok to use goto in catch?

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?

## 5 Answers

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);
}

• Are you sure it's a good idea to rely on the fact that a Message of an exception won't be null or empty? I think using multiple return statements (one in try, one in catch) would be a better solution here. Nov 16, 2012 at 1:33
• @svick valid point. I've updated answer to reflect this. cheers. Nov 16, 2012 at 1:59
• How about limiting the number of attempts? Also, sometimes you should capitalize the s in String. Run StyleCop on your code and you will see. Nov 17, 2012 at 15:15
• @Leonid Fair point. I think it's more of a style (team) preference though so might leave that to the OP to decide. Check this out stackoverflow.com/questions/263191/…. I tend to listen to his suggestions. As for limiting requests. Another good point that the OP can look into if required. Nov 17, 2012 at 18:20
• Oh how I abhor string.Empty. "" is so much shorter and not a jot less clear. But in general, this is a very clean implementation. +1 Nov 18, 2012 at 14:26

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);
}
}
}

• Recursion would be a different approach. Fancy sharing the code? Nov 15, 2012 at 8:31
• I added some sample code. Nov 15, 2012 at 10:08
• Recursion is a dangerous option here. If the problem is persistent you could end up with a stack overflow from repeated attempts. It's not likely in the current form with a user prompt between each cycle; but you're creating a bomb that will be armed when a maintenance developer either replaces the prompt with an automatic retry, or uses this code as a template for an option that never has UI interaction, but doesn't otherwise change the implementation. Nov 15, 2012 at 13:34
• I understand the downvote(s), but the code is still perfectly fine in a simple situation. I added a warning to my answer. Nov 15, 2012 at 22:54
• Good answer. One thing I would add is a number of retries parameter and deal with them tail-recursively. Nov 17, 2012 at 15:16

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.

• What about using goto inside switch, or to get out of deeply nested loops, is it also spaghetti code? Nov 15, 2012 at 9:28
• @ANeves +1 Using a goto once for a specific situation is "ok", but I personally try to find other solutions (exceptions, recursion, refactoring, etc.) first. Nov 15, 2012 at 11:12
• @ANeves - goto case is a perfectly legitimate use of goto. Getting out of a deep loop should be refactored so that you can use a return statement instead, if possible. Nov 15, 2012 at 17:25

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);

• This approach increases the level of complexity (volume of skull sweat required to reason about code that contains such a call) for such a simple problem. Nov 17, 2012 at 15:18
• Asynchronous callbacks are considered very simple to experienced programmers. Novices should learn this pattern. It should be a standard part of a programmers tool box to increase code readability and decrease bugs.
– Greg
Nov 18, 2012 at 11:22
• ("and decrease bugs" would benefit from an example.) I don't like this approach, because if other methods rely on this method's side-effects this approach will bite the programmer's hand. But it's a nice tool to know, yes. Nov 19, 2012 at 9:33
• I think the classic bug it reduces is preventing recursion overflow in an automated retry situation. Using asynchronous calls also frees up other items to render between retries and does not block the UI. I can imagine a situation where a coder does an automated retry every second in a while loop, the code always raises an exception and the user never gets back control of the UI. I agree it could cause problems if following code depended on success. Though, any code depending on the table fill should be placed in the try block.
– Greg
Nov 19, 2012 at 12:14

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.

• "I'm not a fan of the TrySomething with out parameter pattern": why not? If DoSomething returns void, why should TryDoSomething need any out parameter? A concrete example: TrySendEmail(to, message). Nov 19, 2012 at 9:25
• @ANeves: I've updated the answer to explain my reasoning. Nov 19, 2012 at 10:27