Réflexion emboîtée IF déclaration pour plus de clarté

Question

Je veux refactoriser ce mumbo jumbo d'une méthode pour le rendre plus lisible, il a beaucoup de IF imbriqués à mon goût.

Comment voulez-vous refactoriser cela?

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

Je voudrais inverser les conditions dans le test à si mauvais puis supprimerAndLog comme l'exemple ci-dessous. Cela empêche l'imbrication et place l'action près du 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; 
    } 
    ... 

}... 

Answer 2

Une approche possible consiste à avoir des instructions single if qui vérifient lorsque la condition n'est pas vraie. Avoir un retour pour chacun de ces contrôles. Cela transforme votre méthode en une séquence de blocs «si» au lieu d'un nid.

Answer 3

Il n'y a pas beaucoup de choses à factoriser ici, que vous gardez les 3 tests séparément en raison du fait que les messages d'erreur liés au test effectué. Vous pouvez opter pour que les méthodes de test rapportent l'erreur à journaliser afin que vous ne les ayez pas dans l'arbre if/else, ce qui pourrait rendre les choses plus simples car vous pourriez simplement tester une erreur et la logger + supprimer le fichier .

Answer 4

Clauses de protection.

Pour chaque condition, annulez-la, remplacez le bloc else par le bloc then et renvoyez-le.

Ainsi

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

Devient:

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

Answer 5

Si vous n'êtes pas contre l'utilisation des exceptions, vous pourriez gérer les contrôles sans imbrication.

Attention, le code de l'air avant:

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

Il est le elses ci-dessus que jeter mon oeil. Voici un alternatif, à l'intérieur du try {}

Vous pouvez faire cela encore plus court en retournant après MoveToSafeFolder (Même si vous retournez le bloc finally sera exécuté.) Ensuite, vous n'avez pas besoin d'affecter un vide chaîne à errorMessage, et vous n'avez pas besoin de vérifier est errorString vide avant de supprimer le fichier et de consigner le message). Je ne l'ai pas fait ici parce que beaucoup trouvent les retours anticipés offensants, et je serais d'accord dans ce cas, puisque l'exécution du bloc finally après le retour n'est pas intuitive pour beaucoup de gens.

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

Answer 7

plût à quelque chose comme ceci:

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

En réponse David Waters, je n'aime pas le motif DeleteFile LogError répété. Je soit écrire une méthode d'assistance appelée DeleteFileAndLog (fichier chaîne, erreur de chaîne) ou j'écrire le code comme ceci:

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

Comment cela?

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

ou, mieux encore, ... ceci:

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

REMARQUE: Vous ne devriez pas prendras et avaler sans exception générique rethrowing il ...