Can/Should I replace this GOTO statement in C#

Question

using goto seems natural in here.

A project needs to read pdf file, a pdf file can be one of below.

Not Protected
Protected with password1
Protected with password2
Protected with password3

A file can only be accessed with correct password, there is no way to know which password the file need upfront. we have to try all cases, (include no password). if none of above password works, throw exception.

PdfReader GetPdfReader(string filePath)
{
    PdfReader r = null;
    int Retries = 0;
    start: try
    {
        switch (Retries)
        {
            case 0: r = new PdfReader(filePath); break;
            case 1: r = new PdfReader(filePath, password1); break;
            case 2: r = new PdfReader(filePath, password2); break;
            case 3: r = new PdfReader(filePath, password3); break;
        }
    }
    catch (BadPasswordException ex)
    {
        if (Retries == 3) throw ex;
        Retries++;
        goto start;
    }
    return r;
}

Embed try/catch is working but looks ugly, using goto seem natural.

Two questions:

Should I replace this goto?
Is there an elegant way to replace this goto?

Thanks

Yes, you should. Why in the world wouldn't you just use a loop? You're emulating one with a `goto`... in C# — Ed S., Oct 09 '14 at 21:48
Never use `throw ex` to rethrow the exception, instead just use `throw`. `throw ex` will not preserve the original stacktrace where as throw does. It will be very very useful for debuuging. — Sriram Sakthivel, Oct 09 '14 at 21:51
A simple `while (Retries <= 3)` is sufficient to replace the `goto` here. The `switch()` isn't exactly the height of elegance either. — H H, Oct 09 '14 at 21:53
@EdS.: He's not *emulating* a loop, but implementing one. `while` does essentially the same thing. — Eric J., Oct 09 '14 at 21:54
Yes, replace it. It makes no sense __and__ it goes upward. Jumping out of or inside nested loops may be a good thing but here there is no value in the goto. — TaW, Oct 09 '14 at 21:55

Eric J. · Answer 1 · 2014-10-09T21:55:21.933

5

Goto, just like nuclear power or bacon, is not inherently evil. It's a matter of what you do with it.

The current code is fairly clearly structured. Goto is not being used to create spaghetti code.

Having said that, I would still replace it with a while loop. Goto can be a slippery slope.

bool passwordIsOK = false;
while (!passwordIsOK && Retries < 3)
{
    // Existing code, without "goto start;".  Set passwordIsOK = true when appropriate.
    // See also @Sriram's comment about using "throw" rather than "throw ex".
}

edited Oct 09 '14 at 21:55

answered Oct 09 '14 at 21:50

Eric J.

147,927
63
340
553

1

Rather than introducing `passwordIsOk`, I would put a `break;` after the closing brace of the `switch`. – Greg Hewgill Oct 09 '14 at 21:59
@GregHewgill: Also not a bad idea. My personal style is to be rather explicit about what is going on even if it means an extra line of code. I feel that it tends to reduce errors when I or someone else maintain the code years later. Your suggestion is fine in my book. It's clear enough to a reasonable maintainer. – Eric J. Oct 09 '14 at 22:04

score 2 · Accepted Answer · edited Oct 10 '14 at 23:03

I think you should replace the goto in this case as to me the issue is that your code and retries are clouding the intent. I personally would replace the code with something like this:

PdfReader GetPdfReader(string filePath)
{
    PdfReader r = null;

    string [] passwordsToTry = new string [] {null, password1, password2, password3};

    foreach(string password in passwordsToTry)
    {
        try
        {
            r = (password == null) ? 
                new PdfReader(filePath) 
              : new PdfReader(filePath, password);
            if (r != null) 
               break;
        }
        catch(BadPasswordException){ }
    }
    return r;
}

To me the code is clearer as it shows:

You have defined list of passwords to try including 'none'
You don't care about BadPasswordExceptions except to ignore them
If you get a hit then the loop exits
If you get no hits the loop exits at the end

The other thing is that your code with 3 passwords is slightly brittle if you had to deal with more or less passwords. And I think using a variable like passwordsToTry fits nicely with the 'try' statement.

This exits normally if it fails, but according to the question it should rethrow the last exception. — Ben Voigt, Oct 10 '14 at 23:59

score 0 · Answer 3 · edited May 23 '17 at 10:27

You can do a while(true) with a break :

PdfReader GetPdfReader(string filePath)
{
    PdfReader r = null;
    int Retries = 0;
    while(true)
    {
        try
        {
            switch (Retries)
            {
                case 0: r = new PdfReader(filePath); break;
                case 1: r = new PdfReader(filePath, password1); break;
                case 2: r = new PdfReader(filePath, password2); break;
                case 3: r = new PdfReader(filePath, password3); break;
            }
            break;
        }
        catch (BadPasswordException ex)
        {
            if (Retries == 3) throw ex;
            Retries++;
        }
    }
    return r;
}

This avoids adding any extra variables. This topic goes over pros/cons of goto.

Can/Should I replace this GOTO statement in C#

3 Answers3