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:

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

Thanks

Answer 1

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".
}

Answer 2

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:

1. You have defined list of passwords to try including 'none'
2. You don't care about BadPasswordExceptions except to ignore them
3. If you get a hit then the loop exits
4. 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.