为清晰起见重构嵌套的 IF 语句[关闭]答案

【问题标题】：Refactor nested IF statement for clarity [closed]为清晰起见重构嵌套的 IF 语句[关闭]
【发布时间】：2008-12-10 14:00:32
【问题描述】：

我想重构这个笨拙的方法以使其更具可读性，它有很多嵌套的 IF 来满足我的喜好。

你会如何重构它？

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

【问题讨论】：

这是什么家庭作业？欺骗stackoverflow.com/questions/348562/…
负面反馈只会伤害网站，有一天他们可能会接受。无论如何，这是我要问的问题。赞成票。
这个问题似乎是题外话，因为可能属于codereview.stackexchange.com

标签： refactoring coding-style

【解决方案1】：

我会将测试中的条件反转为 if bad then deleteAndLog，如下例所示。这可以防止嵌套并将操作置于测试附近。

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

}...

【讨论】：

这和我的经验法则差不多：“永远不要测试正常情况，测试异常”，stackoverflow.com/questions/114342/…。

【解决方案2】：

保护子句。

对每个条件取反，将else块改为then块，然后返回。

这样

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

变成：

if(!IsValidFileFormat(filename)
{
    // else 
    return;     
}
// then

【讨论】：

【解决方案3】：

如果您不反对使用异常，您可以在不嵌套的情况下处理检查。

警告，前方空气代码：

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

【讨论】：

它更整洁，但是以这种方式使用异常是代码异味（对不起，但确实如此）。另外，为什么要抛出特定类型的异常，当您知道要捕获它们并完全一样对待它们时？
为什么是“代码味道”？顺便说一句，“代码气味”是否意味着“我不喜欢它”？我已经用 Google 搜索过，但我发现唯一与异常相关的“代码味道”是将它们用于非异常情况。
1) 我知道“代码异味”，但在这里我倾向于不同意，我说“如果你不反对它”。我知道异常会触发堆栈展开并且比标志慢 - 但谁在乎，用于文件上传有效性检查。 2) 我知道，不需要输入异常。但这只是一个例子。
代码的味道往往会在旁观者的脑海中浮现。但我不得不同意，这似乎更像是使用异常来控制流，这通常似乎是不受欢迎的。
这取决于语言，我猜。我不知道，也许编译语言的用户比解释语言的用户不喜欢这种用法。我可以理解为什么这种用法一般来说很糟糕，但我不太明白为什么这里这么糟糕。（而且 - 病毒不值得例外吗？）

【解决方案4】：

一种可能的方法是使用单个 if 语句来检查条件何时不成立。对这些支票中的每一项都有回报。这会将您的方法变成一系列“if”块而不是嵌套。

【讨论】：

【解决方案5】：

这里没有太多需要重构的地方，因为错误消息与执行的测试相关，因此您将 3 个测试分开保存。您可以选择让测试方法将错误报告回记录，这样您就不会在 if/else 树中包含它们，这可以使事情变得更简单，因为您可以简单地测试错误并记录它+删除文件.

【讨论】：

【解决方案6】：

在 David Waters 的回复中，我不喜欢重复的 DeleteFile LogError 模式。我要么编写一个名为 DeleteFileAndLog(string file, string error) 的辅助方法，要么编写如下代码：

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

【讨论】：

DeleteFileAndLog 听起来不是一个好名字。它是删除文件和日志（顾名思义）还是删除文件然后记录事件（我假设是这个意思）。

【解决方案7】：

让我眼前一亮的是上面的其他东西。这是 an 替代方法，在 try {}

中

您可以通过在 MoveToSafeFolder 之后返回来缩短此时间（即使您返回 finally 块也会被执行。）然后您不需要为 errorMessage 分配一个空字符串，也不需要检查在删除文件和记录消息之前 errorString 为空）。我没有在这里这样做，因为许多人认为提前返回令人反感，我同意在这种情况下，因为在返回之后执行 finally 块对许多人来说是不直观的。

希望对你有帮助

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

【讨论】：

【解决方案8】：

我想要这样的事情：

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

【讨论】：

【解决方案9】：

这个怎么样？

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

或者，更好的是……这个：

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

注意：你不应该在不重新抛出的情况下捕获和吞下通用异常......

【讨论】：