8

Is there a better way to write this code without using goto? It seems awkward, but I can't think of a better way. I need to be able to perform one retry attempt, but I don't want to duplicate any code.

public void Write(string body)
{
    bool retry = false;
RetryPoint:
    try
    {
        m_Outputfile.Write(body);
        m_Outputfile.Flush();
    }
    catch (Exception)
    {
        if( retry )
            throw; 
        // try to re-open the file...
        m_Outputfile = new StreamWriter(m_Filepath, true);
        retry = true;
        goto RetryPoint;
    }
}
CluelessCoder
  • 193
  • 1
  • 5
  • 11
    sorry, can't resist! http://xkcd.com/292/ –  Nov 10 '10 at 20:44
  • 2
    There's ALWAYS a better way to write logic without a goto. – Paul Sonier Nov 10 '10 at 20:48
  • 1
    @McWafflestix: I disagree. There are some *very rare* cases where using `goto` in fact yields cleaner code -- breaking out of nested loops is a commonly cited example (since C# does not have labeled breaks like Java). See http://stackoverflow.com/questions/2542289/is-there-ever-a-reason-to-use-goto-in-modern-net-code for more. – Heinzi Nov 11 '10 at 23:52
  • @Heinzi: Okay, your point is taken: for certain conditions of very bad code, using goto can yield cleaner code; I'd classify that as a smell, though, and a particularly bad one. – Paul Sonier Nov 12 '10 at 00:52
  • @McWafflestix: And an easily fixed one. It's usually an indicator of excessive nesting depth in a method. – Steven Sudit Nov 12 '10 at 23:18

7 Answers7

20

Here is the basic logic that I would use instead of a goto statement:

bool succeeded = false;
int tries = 2;

do
{
    try
    {
        m_Outputfile = new StreamWriter(m_Filepath, true);
        m_Outputfile.Write(body); 
        m_Outputfile.Flush(); 
        succeeded = true;
    }
    catch(Exception)
    {
        tries--;
    }
}
while (!succeeded && tries > 0);

I just added # of tries logic, even though the original question didn't have any.

Leon Cullens
  • 12,276
  • 10
  • 51
  • 85
Michael S. Scherotter
  • 10,715
  • 3
  • 34
  • 57
  • This performs infinite tries, uses C++ syntax for the catch, and doesn't rethrow. I have no idea why it was upvoted when it's clearly wrong. – Steven Sudit Nov 10 '10 at 20:55
  • Oh, and it doesn't reopen on failure. – Steven Sudit Nov 10 '10 at 20:57
  • 2
    @Steven Sudit: The body of the catch block is implied to be the same as the original code. – LBushkin Nov 10 '10 at 21:04
  • @LBushkin, not without some changes. The original catch block won't even compile in this code. In any case, this still retries forever, which is *not* the goal. – Steven Sudit Nov 10 '10 at 21:08
  • 1
    The catch block needs to `Close` the streamwriter! – Steven Sudit Nov 11 '10 at 15:38
  • Thank you for fixing the catch block syntax, but I don't see why a `do`/`while` would be preferable to a `for`/`break`. It requires an additional variable and more lines of code. – Steven Sudit Nov 11 '10 at 15:39
  • Hmm, this doesn't rethrow on the last failure, either. I'm sorry, but no matter how I look at it, even the newest version of this answer has multiple errors. It was a mistake to mark it Accepted. – Steven Sudit Nov 11 '10 at 15:40
5

Michael's solution doesn't quite fulfill the requirements, which are to retry a fixed number of times, throwing the last failure.

For this, I would recommend a simple for loop, counting down. If you succeed, exit with break (or, if convenient, return). Otherwise, let the catch check to see if the index is down to 0. If so, rethrow instead of logging or ignoring.

public void Write(string body, bool retryOnError)
{
    for (int tries = MaxRetries; tries >= 0; tries--)
    {
        try
        {
            _outputfile.Write(body);
            _outputfile.Flush();
            break;
        }
        catch (Exception)
        {
            if (tries == 0)
                throw; 

            _outputfile.Close();
            _outputfile = new StreamWriter(_filepath, true);
        }
    }
}

In the example above, a return would have been fine, but I wanted to show the general case.

Steven Sudit
  • 19,391
  • 1
  • 51
  • 53
  • @Anthony: Thank you, that was a mistake. I'll fix it immediately. – Steven Sudit Nov 10 '10 at 21:08
  • I just made one more change that I think is applicable to any solution: I made it `Close` the old streamwriter before opening a new one. If this isn't done, then the old one would keep a handle open until GC kicks in, blocking new ones from working! – Steven Sudit Nov 11 '10 at 15:38
5

@Michael's answer (with a correctly implemented out catch block) is probably the easiest to use in your case, and is the simplest. But in the interest of presenting alternatives, here is a version that factors the "retry" flow control into a separate method:

// define a flow control method that performs an action, with an optional retry
public static void WithRetry( Action action, Action recovery )
{
    try {
        action(); 
    }
    catch (Exception) {
        recovery();
        action();
    }
}

public void Send(string body)
{
    WithRetry(() =>
    // action logic:
    {
       m_Outputfile.Write(body);
       m_Outputfile.Flush();
    },
    // retry logic:
    () =>
    {
       m_Outputfile = new StreamWriter(m_Filepath, true);
    });
}

You could, of course, improve this with things like retry counts, better error propagation, and so on.

Community
  • 1
  • 1
LBushkin
  • 129,300
  • 32
  • 216
  • 265
  • I'm really not sure what to make of this. Arguably, this sort of delegate-driven solution is elegant, flexible and reusable. Then again, it currently doesn't actually do what's needed. In the end, I'm going to neither up- nor down-vote. – Steven Sudit Nov 10 '10 at 21:10
  • @Steven: Did I miss something? In what way does it not fulfill the requirements? – LBushkin Nov 10 '10 at 21:12
  • The ways you mentioned: doesn't have a retry count, doesn't throw on last try, etc. No doubt you can *make* it do what's needed -- I've seen enough of your answers to be certain of that -- but you haven't at this time. – Steven Sudit Nov 10 '10 at 21:13
  • 2
    @Steven: Well, in all fairness, the OP never mentioned a need to perform multiple retries, and in fact, as the original code is written it will only perform a retry once. As for throwing on the last attempt, the code should do that, since the action delegate is re-invoked in the catch block. – LBushkin Nov 10 '10 at 21:15
  • If all we want to do is retry once, we can skip all the delegates and loops, instead hard-coding it. I would hope that we can do better than this sort of brute-force solution, particularly given how it fails for larger numbers of retries. – Steven Sudit Nov 10 '10 at 21:20
  • @Heinzi, I'll easily grant that it's both creative and technically sophisticated. I'm just not sure if it's appropriate. – Steven Sudit Nov 11 '10 at 18:26
  • @LBushkin, I was reminded that I had actually used code that's a bit like yours. I had a method call Retry that took an action delegate and a retry count. It would ignore the first n-1 failures, retrying the code in the delegate. Best of both worlds, really. – Steven Sudit Nov 11 '10 at 18:35
  • @Steven: True, but even though it might not be the best solution for the exact problem of the OP, it's definitely a useful and instructive answer, hence the upvote. – Heinzi Nov 11 '10 at 23:44
  • @Heinzi, yes, I eventually upvoted him, and you. – Steven Sudit Nov 11 '10 at 23:54
1

What if you put it in a loop? Something similar to this, maybe.

while(tryToOpenFile)
{
    try
    {
        //some code
    }
    catch
    {
    }
    finally
    {
        //set tryToOpenFile to false when you need to break
    }
}
1
public void Write(string body, bool retryOnError)
{
    try
    {
        m_Outputfile.Write(body);
        m_Outputfile.Flush();
    }
    catch (Exception)
    {
        if(!retryOnError)
            throw; 
        // try to re-open the file...
        m_Outputfile = new StreamWriter(m_Filepath, true);
        Write(body, false);
    }
}
Heinzi
  • 167,459
  • 57
  • 363
  • 519
  • @Steven: Sure, why not? It's a simple tail recursion and very readable: `Write(body, false)` quite clearly documents the writer's intention (try the same again but do not retry) and it avoids cluttering the code (no loop, no `succeeded` variable). Replacing `retryOnError` with a retry count is also an easy exercise... – Heinzi Nov 11 '10 at 06:39
  • A few reasons. Due to the way GC works, recursion keeps objects alive longer, so it should be avoided when an iterative solution is sufficiently clear. When the tail recursion optimization is possible, this is less of an issue. This example recurses no more than once, but you would change the bool to a retry countdown if you wanted to increase that (and you likely would). Such a change would magnify the effects. – Steven Sudit Nov 11 '10 at 15:33
  • But aside from this optimization issue, my main concern is that this problem does not lend itself particularly well to a recursive solution, so the code is going to be harder to understand and maintain. The iterative version makes its loop explicit instead of expecting the reader to notice that the third "Write" they see is actually a recursive call. A bool flag isn't needed, regardless, since the retry countdown doubles as an indicator of when we should rethrow. – Steven Sudit Nov 11 '10 at 15:36
  • @Steven: True, it's definitely not one of those examples where recursion beats iteration (and +1 to your iterative solution without the flag, BTW). I still consider it a valid (and hopefully instructive) alternative, so I won't delete the answer. One advantage I see in the recursive solution is that the error-handling code is completely inside the catch clause (except for the additional parameter), so the code represents the *natural (non-exceptional) flow* of the program. The iterative solution, on the other hand, starts with a big loop which is only used in *exceptional* circumstances. – Heinzi Nov 11 '10 at 17:57
  • I would not suggest deleting your answer! It's much like LBushkin's, in that it represents a unique approach that is valuable educationally, even if it's not necessarily the very best fit for the problem. My criticism is intended to be entirely constructive. Good point about keeping the exceptional code in the exception handler. My *fancy* answer to that, which I've actually used in production, is to use a delegate-based retrier, like LBushkin's. This hides away the guts of the retry very cleanly, and makes it possible to filter out fatal exception types (StackOverflow, ThreadAbort, etc.). – Steven Sudit Nov 11 '10 at 18:31
0

Try something like the following:

int tryCount = 0;
bool succeeded = false;

while(!succeeded && tryCount<2){
    tryCount++;
    try{
        //interesting stuff here that may fail.

        succeeded=true;
    } catch {
    }
}
chezy525
  • 4,025
  • 6
  • 28
  • 41
0

with a boolean

public void Write(string body)
{
        bool NotFailedOnce = true;            
        while (true)
        {
            try
            {
                 _outputfile.Write(body);
                 _outputfile.Flush();           
                 return;                    
            }
            catch (Exception)
            {
                NotFailedOnce = !NotFailedOnce;
                if (NotFailedOnce)
                {
                    throw;
                }
                else
                {
                     m_Outputfile = new StreamWriter(m_Filepath, true);
                }
            }
      }        
}
Jimmy
  • 3,224
  • 5
  • 29
  • 47
  • This can't possible work. For one thing, `NotFailedOnce = !NotFailedOnce)` will always be false, so it'll never throw. Instead, it'll loop forever. – Steven Sudit Nov 11 '10 at 04:49
  • @Steve, look at the code again, it is not an equality check, but an assignment. – Jimmy Nov 11 '10 at 09:48
  • that's clever, meaning bad. The compiler will generate warnings for that, and it ought to, since an assignment in the middle of a predicate is most likely due to a typo. Likewise, it's clever/bad to set a bool to false by NOTing it instead of, you know, setting it to false. This code is unnecessarily obfuscated. – Steven Sudit Nov 11 '10 at 15:29
  • @Steven- agree with the point of assignment in the if(), I changed that. However, NOTing is completely different from setting it to false. It is the NOTing which will make sure it works only once. – Jimmy Nov 11 '10 at 17:29
  • Thanks for moving the assignment out of the predicate. If you look at the flow, you'll see that you could have had a `HasFailed`, instead. It would be set right above the line that instantiates a streamwriter. Much simpler that way. Also, there's no need for an else (or all those braces), as a throw exits the current flow. – Steven Sudit Nov 11 '10 at 18:33