Help refactor this messed-up-nested-if of a method

Question

Ok I want some opinions how I can fix this mess of a method!

It has WAY to many nested 'if' statements.

But realize I have to know exactly where the method fails, currently in each of the respective 'else' clause I am logging the error (the failed 'if' condition').

Note: ignore any logic behind the things, please focus on the style and structure as I have made all the function names up etc.

Here is the skeleton structure:

   public void MyMethod()
{

   try
   {
    bool tryAgain = false;

    string filename = DownloadFile();

    if( IsFileFormatOk(filename) )
    {

        blah = GetBlah(filename);

        if(blah.ID > 0)
        {

            if(ImportFile(filename)
            {

                string username = GetUserFromFile(filename);

                if(isValidUser(username))
                {

                    // few more levels to go
                    //
                    //
                    //

                }
                else
                {
                    LogError(filename, ...); // specific to this if statement
                    tryAgain = true;
                }


            }
            else
            {

                LogError(filename, ...); // specific to this if statement
                tryAgain = true;
            }

        }
        else
        {
            LogError(filename, ...); // specific to this if statement
            tryAgain = true;
        }

    }
    else
    {
        LogError(filename, ...); // specific to this if statement
        tryAgain = true;
    }

   }
   catch
   {
   }
   finally
   {
    if(tryAgain)
    {
        // blah
    }
   }


}

Answer 1

I would work on changing your logic so you can return from the method as soon as possible instead of nesting more logic. Fore example:

//  GOOD
if (!file.exists())
    return;
// Rest of the code

// BAD
if (file.exists()){
    // Code goes here
}
return;

This may help remove some nesting and make things less complicated.

Answer 2

I would guess there's quite a bit of logic that's waiting to be extracted elsewhere, but in any case here's another way to flatten the nesting:

try
{
  if( !IsFileFormatOk(filename) )
    throw new MySpecificException(...); // pass specific log params

  blah = GetBlah(filename);

  if(blah.ID <= 0)
    throw new MySpecificException(...); // pass specific log params

  if(!ImportFile(filename)
    throw new MySpecificException(...); // pass specific log params

  string username = GetUserFromFile(filename);

  // ...
}
catch (MySecificException e)
{
  LogError(filename, e.LogParams)
  // blah
}

Answer 3

You can start by putting the toplevel if (true branch) in a separate method. And continue until satisfied.

So generally you change:

if (condition) {
  // Block1
} else {
  // Block2
}

Into

if (condition) {
  Block(...);
} else {
  Block2(...);
}

Please beware of the local variables that need to be passed tot the new method.

Answer 4

You should try having all of the methods that you are calling throw an exception in case of an error. The exceptions thrown should contain whatever error message you wish to log. Create a LogError() method which takes any exception and logs the embedded message. Like this (pseudo-code):

MyMethod()
{
  try
  {
    string filename = DownloadFile()
    blah = GetBlah(filename)
    ImportFile(filename)
    ...
  }
  catch DownloadFileException, GetBlahException, ImportFileException e
  {
    LogError(e)
  }
}

Of course, you will have to create all of those Exceptions, but that is pretty trivial. In most languages you just subclass whatever the top level Exception object is.

Answer 5

Edit: Now that I know there are so many levels, we need to take another approach. I don't know what's in the "try again" bit, but you need to change around the logic so it tests for a failure case, not a success case. If it fails, log and return (or somehow do whatever try again entails). Otherwise, you can keep going outside the if.

if(EndOfWorld)
{
    WriteLastLogEntryEver();
    return; //run away
}

//we're safe (for now)
ChargeOnAhead();

---

Before I knew there were many more levels of nested if, I suggested the following.

public void MyMethod()
{

   try
   {
    bool tryAgain = false;

    string filename = DownloadFile();

    if( IsFileFormatOk(filename) )
    {

        tryAgain = DoBlah(filename, ...);
    }
    else
    {
        LogError(filename, ...); // specific to this if statement
        tryAgain = true;
    }

   }
   catch
   {
   }
   finally
   {
    if(tryAgain)
    {
        // blah
    }
   }


}

private bool DoImport(string filename, blah)
{

    if(ImportFile(filename))
    {

            // and so forth!
            return false;
    }

    LogError(filename, ...); // specific to this if statement
    return true;
}

private bool DoBlah(string filename)
{
        blah = GetBlah(filename);

        if(blah.ID > 0)
        {

            return DoImport(filename, ...);

        }

        LogError(filename, ...); // specific to this if statement
        return true;

}

I'm sure you could refactor this a bit more and pull the // and so forth into a new method as well. It depends on how much stuff you're going to have to pass back and forth. And if you're passing too much, consider if it makes more sense to be a parameter or a class field.

---

Answer 6

Here's jjnguy's suggestion, implemented:

public void MyMethod() {
    try
    {
        bool tryAgain = false;
        string filename = DownloadFile();

        if( !IsFileFormatOk(filename) )
        {
            LogError(filename, ...); // specific to this if statement
            tryAgain = true;
            return;
        }

        blah = GetBlah(filename);
        if(blah.ID <= 0)
        {
            LogError(filename, ...); // specific to this if statement
            tryAgain = true;
            return;
        }

        if(!ImportFile(filename))
        {
            LogError(filename, ...); // specific to this if statement
            tryAgain = true;
            return;
        }

        string username = GetUserFromFile(filename);

        if(!isValidUser(username))
        {
            LogError(filename, ...); // specific to this if statement
            tryAgain = true;
            return
        }

        // few more levels to go
        //

    finally
    {
        if(tryAgain)
        {
            // blah
        }
   }
}

Answer 7

For these kind of questions, http://refactormycode.com/ is very useful.

Although the SO people are really committed to refactoring some code, too. ;)