Refactor verschachtelte IF-Anweisung für Klarheit

Question

Ich möchte diesen Hokuspokus einer Methode umgestalten, um es lesbarer zu machen, es hat viel zu viele verschachtelte IF's für meinen Geschmack.

Wie würden Sie dies umgestalten?

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

Ich würde die Bedingungen im Test zu umkehren, wenn schlecht dann deleteAndLog wie das Beispiel unten. Dies verhindert das Verschachteln und bringt die Aktion in die Nähe des Tests.

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

Ein möglicher Ansatz besteht darin, einzelne if-Anweisungen zu verwenden, die prüfen, ob die Bedingung nicht erfüllt ist. Haben Sie eine Rückkehr für jede dieser Prüfungen. Dies verwandelt Ihre Methode in eine Sequenz von 'if' Blöcken anstelle von einem Nest.

Answer 3

Es gibt nicht viel zu hier Refactoring, wie Sie die drei Tests getrennt halten aufgrund der Tatsache, dass die Fehlermeldungen zum Test beziehen durchgeführt. Sie können sich dafür entscheiden, dass die Testmethoden den Fehler zur Protokollierung zurückmelden, damit Sie sie nicht in der if/else-Struktur haben, was die Dinge einfacher machen könnte, da Sie dann einfach nach einem Fehler suchen und ihn protokollieren + die Datei löschen können .

Answer 4

Schutzklauseln.

Für jede Bedingung, negieren Sie sie, ändern Sie den Else-Block in den Dann-Block und kehren Sie zurück.

So

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

Becomes:

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

Answer 5

Wenn Sie nicht gegen die Verwendung von Ausnahmen sind, könnten Sie die Schecks handhaben, ohne nisten.

Warnung, Luft Code vor:

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

Es ist die elses oben, dass mein Auge werfen. Hier ist eine Alternative innerhalb des try {}

Sie diese noch kürzer machen können nach MoveToSafeFolder durch Rücksendung (Auch wenn Sie der finally-Block sind Rückkehr wird ausgeführt.) Dann sind Sie nicht eine leere zuweisen müssen string to errorMessage, und Sie müssen nicht überprüfen, ob errorString leer ist, bevor Sie die Datei löschen und die Nachricht protokollieren. Ich habe es hier nicht gemacht, weil viele frühe Offensive finden, und ich würde in diesem Fall zustimmen, da die Ausführung des finally-Blocks nach der Rückkehr für viele Leute nicht intuitiv ist.

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

Answer 7

ich so etwas wie dies würde:

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

In David Waters Antwort, ich weiß nicht wie die wiederholten DeleteFile LogError Muster. Ich würde entweder eine Hilfsmethode namens DeleteFileAndLog (string Datei, string Fehler) schreiben oder ich würde den Code wie folgt schreiben:

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

Wie wäre das?

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

oder, noch besser, ... dies:

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

Hinweis: Sie sollten nicht zu kontrollieren und allgemeine Ausnahme schlucken, ohne es zu Erneutes Auslösen ...