Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
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?
share|improve this question

5 Answers 5

up vote 30 down vote accepted

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);
}
share|improve this answer
    
It looks much better, thanks. –  disbeliever Nov 15 '12 at 9:12
    
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. –  svick Nov 16 '12 at 1:33
    
@svick valid point. I've updated answer to reflect this. cheers. –  dreza Nov 16 '12 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. –  Leonid Nov 17 '12 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. –  dreza Nov 17 '12 at 18:20

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);
        }
    }
}
share|improve this answer
2  
Recursion would be a different approach. Fancy sharing the code? –  dreza Nov 15 '12 at 8:31
    
I added some sample code. –  Jens Mühlenhoff Nov 15 '12 at 10:08
3  
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. –  Dan Neely Nov 15 '12 at 13:34
2  
I understand the downvote(s), but the code is still perfectly fine in a simple situation. I added a warning to my answer. –  Jens Mühlenhoff Nov 15 '12 at 22:54
3  
Good answer. One thing I would add is a number of retries parameter and deal with them tail-recursively. –  Leonid Nov 17 '12 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.

share|improve this answer
3  
What about using goto inside switch, or to get out of deeply nested loops, is it also spaghetti code? –  ANeves Nov 15 '12 at 9:28
3  
@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. –  Jens Mühlenhoff Nov 15 '12 at 11:12
3  
@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. –  Bobson Nov 15 '12 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);
share|improve this answer
1  
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. –  Leonid Nov 17 '12 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 '12 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. –  ANeves Nov 19 '12 at 9:33
1  
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 '12 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.

share|improve this answer
    
"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). –  ANeves Nov 19 '12 at 9:25
    
@ANeves: I've updated the answer to explain my reasoning. –  Anton Golov Nov 19 '12 at 10:27

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.