Clever Geek Handbook

Can / Should I replace this GOTO statement in C #

Using goto seems natural here .

The project needs to read the pdf file, the pdf file can be one of below.

  • Not protected
  • Password protected1
  • Password protected2
  • Password protected3

The file can only be accessed with the correct password, and there is no way to know what password it needs. we have to try all cases (no password). if none of the above passwords work, throw an 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;
}

Insert try / catch works, but looks ugly, using goto seems natural.

Two questions:

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

thank

+3


source to share


3 answers


I think you should replace goto in this case as the problem is that your code and retries cloud the intent. I would personally replace the code with the following:

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

The code is clearer to me, as it shows:



  • You have defined a list of passwords to try enabling 'none'
  • You don't need BadPasswordExceptions values ​​other than ignore them.
  • If you get hit the cycle goes out
  • If you don't get hits, the cycle ends at the end

Another thing is that your code with three passwords is a little fragile if you have to deal with more or less passwords. And I think that using a variable like passwordsToTry fits nicely into the "try" statement.

+2


source


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 well structured. Goto is not used to generate spaghetti code .



Having said that, I would still replace it with a loop while

. 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 comment about using "throw" rather than "throw ex".
}
+4


source


You can do while(true)

with 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 additional variables. This section discusses the pros and cons goto

.

0


source






More articles:


All Articles