views:

340

answers:

7

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


}
+1  A: 

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.

Gamecat
but the code in the Block will have another true and false branch??
Blankman
+4  A: 

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.

jjnguy
A: 

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.


lc
A: 

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.

Chris Lawlor
Exceptions should only be used for *exceptional* behaviour! An expected error condition is not an exception, and as such should be handled without recourse to exceptions.
ZombieSheep
It's still possible to know which calls can throw which exceptions. I don't understand the problem.
recursive
Worst way of using exceptions. Changing if-else for exceptions is a bomb waiting to explode.
Ubersoldat
@ ZombieSheep, Ubersoldat,Not if the if/then logic is actually checking for expeptional behavior, as originally appeared to me. The empty 'catch' block was a red flag. However, the question has been edited since I wrote my response, and it now appears that using exceptions would not be appropriate.
Chris Lawlor
+3  A: 

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
}
orip
A: 

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
        }
   }
}
recursive
A: 

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

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

blindgaenger