views:

1206

answers:

9

Hi,

I want to refactor this mumbo jumbo of a method to make it more readible, it has way to many nested IF's for my liking.

How would you refactor this?

public static void HandleUploadedFile(string filename)
{
  try
  {
    if(IsValidFileFormat(filename)
    {
      int folderID = GetFolderIDFromFilename(filename);
      if(folderID > 0)
      {
        if(HasNoViruses(filename)
        {
          if(VerifyFileSize(filename)
          {
            // file is OK
            MoveToSafeFolder(filename);
          }
          else
          {
            DeleteFile(filename);
            LogError("file size invalid");
          }
        }
        else
        {
          DeleteFile(filename);
          LogError("failed virus test");
        }
      }
      else
      {
        DeleteFile(filename);
        LogError("invalid folder ID");
      }
    }
    else
    {
      DeleteFile(filename);
      LogError("invalid file format");
    }
  }
  catch (Exception ex)
  {
    LogError("unknown error", ex.Message);
  }
  finally
  {
    // do some things
  }
}
+1  A: 

One possible approach is to have single if statements that check for when the condition isn't true. Have a return for each one of these checks. This turns your method into a sequence of 'if' blocks instead of a nest.

j0rd4n
+17  A: 

I would reverse the conditions in the test to if bad then deleteAndLog as the example below. This prevent nesting and puts the action near the test.

try{
    if(IsValidFileFormat(filename) == false){
        DeleteFile(filename);
        LogError("invalid file format");
        return;
    }

    int folderID = GetFolderIDFromFilename(filename);
    if(folderID <= 0){
        DeleteFile(filename);
        LogError("invalid folder ID");
        return;
    }
    ...

}...
David Waters
this looks right
Shawn Simon
This is more or less the same as my rule of thumb: "Never test the normal case, test exceptions", http://stackoverflow.com/questions/114342/what-are-code-smells-what-is-the-best-way-to-correct-them/223881#223881 .
hlovdal
+1  A: 

There's not a lot to refactor here, as you keep the 3 tests separately due to the fact that the error messages relate to the test performed. You could opt for having the test methods report back the error to log so you don't have them in the if/else tree, which could make things simpler abit as you then could simply test for an error and log it + delete the file.

Frans Bouma
+8  A: 

Guard clauses.

For each condition, negate it, change the else block into the then block, and return.

Thus

if(IsValidFileFormat(filename)
{
   // then
}
else
{
   // else
}

Becomes:

if(!IsValidFileFormat(filename)
{
    // else 
    return;     
}
// then
jamesh
+1  A: 

If you are not against using exceptions, you could handle the checks without nesting.

Warning, air code ahead:

public static void HandleUploadedFile(string filename)
{
  try
  {
    int folderID = GetFolderIDFromFilename(filename);

    if (folderID == 0)
      throw new InvalidFolderException("invalid folder ID");

    if (!IsValidFileFormat(filename))
      throw new InvalidFileException("invalid file format!");

    if (!HasNoViruses(filename))
      throw new VirusFoundException("failed virus test!");

    if (!VerifyFileSize(filename))
      throw new InvalidFileSizeException("file size invalid");

    // file is OK
    MoveToSafeFolder(filename);
  }
  catch (Exception ex)
  {
    DeleteFile(filename);
    LogError(ex.message);
  }
  finally
  {
    // do some things
  }
}
Tomalak
It's neater, but using exceptions in this way is a code smell (sorry, but it is). Also, why throw specificly typed exceptions, when YOU KNOW you're going to catch them and treat them all exactly the same?
Binary Worrier
Why is it a "code smell"? BTW Does "code smell" mean anything more than "I don't like it"? I have Googled but the only "code smell" relating to exceptions I have found is using them for non-exceptional circumstances.
Tony Andrews
1) I know about the "code smell", but I tend to disagree here, and I said "if you are not against it". I know that exceptions trigger stack unwinding and are slower than flags - but who cares, for a file upload validity check. 2) No need for typed exceptions, I know. But it's just an example.
Tomalak
Code smell often can be in the mind of the beholder. But I have to agree that this seems more like using exceptions for flow control which in general seems to be frowned on.
Torlack
This depends on the language, I guess. Maybe more users of compiled languages dislike this kind of usage than do users of interpreted languages, I don't know. I can see why such usage is bad *in general*, but I don't quite see why this is so bad *here*. (And - is a virus not worth an exception?)
Tomalak
I agree with Tony Andrews - and I would add this is using exceptions for non-exceptional circumstances. I would add that throwing and catching an exception in the same method gets a bit code smell to me.
David Waters
How comes nobody seems to read the "If you are not against using exceptions" right at the start of the answer?
Tomalak
David - "this is using exceptions for non-exceptional circumstances". Is it? Isn't the normal case that an uploaded file won't have a virus, be of invalid size etc.? The uploaded files that fail these checks will be the, er, exceptions?
Tony Andrews
A: 

It's the elses above that throw my eye. Here's an alternative, inside the try {}

You can make this even shorter by returning after MoveToSafeFolder (Even though you're returning the finally block will be executed.) Then you don't need to assign an empty string to errorMessage, and you don't need to check is errorString empty before deleting the file and logging the message). I didn't do it here because many find early returns offensive, and I'd agree in this instance, since having the finally block execute after the return is unintuitive for many people.

Hope this helps

            string errorMessage = "invalid file format";
            if (IsValidFileFormat(filename))
            {
                errorMessage = "invalid folder ID";
                int folderID = GetFolderIDFromFilename(filename);
                if (folderID > 0)
                {
                    errorMessage = "failed virus test";
                    if (HasNoViruses(filename))
                    {
                        errorMessage = "file size invalid";
                        if (VerifyFileSize(filename))
                        {
                            // file is OK                                        
                            MoveToSafeFolder(filename);
                            errorMessage = "";
                        }
                    }
                }
            }
            if (!string.IsNullOrEmpty(errorMessage))
            {
                DeleteFile(filename);
                LogError(errorMessage);
            }
Binary Worrier
A: 

I would to something like this:

public enum FileStates {

MoveToSafeFolder = 1,

InvalidFileSize = 2,

FailedVirusTest = 3,

InvalidFolderID = 4,

InvalidFileFormat = 5,
}


public static void HandleUploadedFile(string filename) {
    try {
        switch (Handledoc(filename)) {
            case FileStates.FailedVirusTest:
                deletefile(filename);
                logerror("Virus");
                break;
            case FileStates.InvalidFileFormat:
                deletefile(filename);
                logerror("Invalid File format");
                break;
            case FileStates.InvalidFileSize:
                deletefile(filename);
                logerror("Invalid File Size");
                break;
            case FileStates.InvalidFolderID:
                deletefile(filename);
                logerror("Invalid Folder ID");
                break;
            case FileStates.MoveToSafeFolder:
                MoveToSafeFolder(filename);
                break;
        }
    }
    catch (Exception ex) {
        logerror("unknown error", ex.Message);
    }
}

private static FileStates Handledoc(string filename) {
    if (isvalidfileformat(filename)) {
        return FileStates.InvalidFileFormat;
    }
    if ((getfolderidfromfilename(filename) <= 0)) {
        return FileStates.InvalidFolderID;
    }
    if ((HasNoViruses(filename) == false)) {
        return FileStates.FailedVirusTest;
    }
    if ((VerifyFileSize(filename) == false)) {
        return FileStates.InvalidFileSize;
    }
    return FileStates.MoveToSafeFolder;
}
Stefan
+1  A: 

In David Waters reply, I don't like the repeated DeleteFile LogError pattern. I would either write a helper method called DeleteFileAndLog(string file, string error) or I would write the code like this:

public static void HandleUploadedFile(string filename)
{
    try
    {
        string errorMessage = TestForInvalidFile(filename);
        if (errorMessage != null)
        {
             LogError(errorMessage);
             DeleteFile(filename);
        }
        else
        {
            MoveToSafeFolder(filename);
        }
    }
    catch (Exception err)
    {
        LogError(err.Message);
        DeleteFile(filename);
    }
    finally { /* */ }
}

private static string TestForInvalidFile(filename)
{
    if (!IsValidFormat(filename))
        return "invalid file format.";
    if (!IsValidFolder(filename))
        return "invalid folder.";
    if (!IsVirusFree(filename))
        return "has viruses";
    if (!IsValidSize(filename))
        return "invalid size.";
    // ... etc ...
    return null;
 }
plinth
DeleteFileAndLog does not sound like a very good name. Does it delete the file and the log (the name implies so) or does it delete the file and then log the event (which I assume is meant).
hlovdal
A: 

How about this?

public static void HandleUploadedFile(string filename)
{  
   try  
   {    
       if(!IsValidFileFormat(filename))        
       { DeleteAndLog(filename, "invalid file format"); return; }
       if(GetFolderIDFromFilename(filename)==0) 
       { DeleteAndLog(filename, "invalid folder ID");   return; }
       if(!HasNoViruses(filename))             
       { DeleteAndLog(filename, "failed virus test");   return; }
       if(!!VerifyFileSize(filename))          
       { DeleteAndLog(filename, "file size invalid");   return; }
       // --------------------------------------------------------
       MoveToSafeFolder(filename); 
   }
   catch (Exception ex)  { LogError("unknown error", ex.Message); throw; }
   finally {    // do some things  }
}     
private void DeleteAndLog(string fileName, string logMessage)
{
    DeleteFile(fileName);
    LogError(logMessage));
}

or, even better, ... this:

public static void HandleUploadedFile(string filename)
{  
   try  
   {    
       if(ValidateUploadedFile(filename))       
           MoveToSafeFolder(filename); 
   }
   catch (Exception ex)  { LogError("unknown error", ex.Message); throw; }
   finally {    // do some things  }
} 
private bool ValidateUploadedFile(string fileName)
{
   if(!IsValidFileFormat(filename))        
    { DeleteAndLog(filename, "invalid file format"); return false; }
   if(GetFolderIDFromFilename(filename)==0) 
    { DeleteAndLog(filename, "invalid folder ID");   return false; }
   if(!HasNoViruses(filename))             
    { DeleteAndLog(filename, "failed virus test");   return false; }
   if(!!VerifyFileSize(filename))          
    { DeleteAndLog(filename, "file size invalid");   return false; }
   // ---------------------------------------------------------------
   return true;

}    
private void DeleteAndLog(string fileName, string logMessage)
{
    DeleteFile(fileName);
    LogError(logMessage));
}

NOTE: You shouldn't be catching and swallowing generic Exception without rethrowing it...

Charles Bretana