Refactor вложенный оператор IF для ясности

Question

Я хочу реорганизовать этот mumbo jumbo метода, чтобы сделать его более читаемым, у него есть путь для многих вложенных 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 
    } 
} 

Answer 1

Я бы сменил условия в тесте на , если это плохо, тогда 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; 
    } 
    ... 

}... 

Answer 2

Один из возможных подходов состоит в том, чтобы иметь одинарные операторы if, которые проверяют, когда условие не является истинным. Имейте возврат для каждой из этих проверок. Это превращает ваш метод в последовательность блоков «if» вместо гнезда.

Answer 3

Там не много, чтобы реорганизовать здесь, как вы держите 3 теста отдельно в связи с тем, что сообщения об ошибках относятся к испытанию. Вы можете выбрать, чтобы методы тестирования сообщали об ошибке в журнал, чтобы вы не имели их в дереве if/else, что упростило бы выполнение abit, поскольку тогда вы могли просто протестировать ошибку и зарегистрировать ее + удалить файл ,

Answer 4

Оговорки охраны.

Для каждого условия, отмените его, замените блок else на блок then и верните.

Таким образом

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

становится:

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

Answer 5

Если вы не против использования исключений, вы могли бы обрабатывать чеки без вложенности.

Внимание, воздух код впереди:

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

Answer 6

Это выше, что вернулись на Родину бросить свой глаз. Вот альтернатива, внутри попытки {}

Вы можете сделать это еще короче, вернувшись после MoveToSafeFolder (Несмотря на то, что возвращается, наконец, блок будет выполнен.) Тогда вам не нужно назначить пустой string в 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); 
      } 

Answer 7

я бы что-то вроде этого:

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

Answer 8

В David Waters ответа, я не люблю повторяющийся рисунок DeleteFile LogError. Я бы либо написать вспомогательный метод под названием DeleteFileAndLog (строка файла, строка ошибки) или я бы написать такой код:

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

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

Примечание: Вы не должны ловить и глотать родовое Exception без Повторное выбрасывание его ...