2008-12-08 6 views
3

J'ai une fonction compliquée qui a besoin d'être refactorisée, il y a trop d'IF imbriquées et ça me rend nerveux juste de la regarder!Refactoriser cette fonction IF imbriquée qui est enveloppée dans un try/catch

S'il vous plaît ignorer ce que les fonctions font, je suis plus préoccupé par la structure/flux et comment il peut être refondus il a moins imbriqué instructions IF

Le débit de base est la suivante:

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


       } 
       else 
       { 
        DeleteFile(filename); 
       } 


      } 
      else 
      { 
       DeleteFile(filename); 
      } 



     } 
     else 
     { 
      DeleteFile(filename); 
     } 
     } 
     catch (Exception ex) 
     { 

     } 
     finally 
     { 
     // do some things 
     } 


} 

Répondre

10

Je serais tenté d'aller aussi loin que:

private static bool CanMoveToSafeFolder(string filename) 
    { 
     return IsValidFileFormat(filename) 
      && GetFolderIDFromFilename(filename) > 0 
      && HasNoViruses(filename) 
      && VerifyFileSize(filename); 
    } 

    public static void HandleUploadedFile(string filename) 
    { 

     try 
     { 

      if (CanMoveToSafeFolder(filename)) 
      { 
       // file is OK 
       MoveToSafeFolder(filename); 
      } 
      else 
      { 
       DeleteFile(filename); 
      } 
     } 
     catch (Exception ex) 
     { 

     } 
     finally 
     { 
      // do some things 
     } 

    } 
+0

Dans le code d'origine, si IsValidFileFormat était faux, le fichier a été supprimé. Ce n'est pas équivalent. –

+0

Désolé - c'est ce que j'ai pour copier et coller sans lire correctement. Je l'ai réparé. –

+0

+1 alors, pour montrer le refactoring "sous-programme d'extraction". –

3

Ce qui suit est équivalent à votre code d'origine.

public static void HandleUploadedFile(string filename) 
{ 
    try 
    { 
     if(IsValidFileFormat(filename) && 
      (GetFolderIDFromFilename(filename) > 0) && 
      HasNoViruses(filename) && 
      VerifyFileSize(filename)) 
     { 
       MoveToSafeFolder(filename); 
     } 
     else 
     { 
       DeleteFile(filename); 
     } 
    } 
    catch (Exception ex) 
    { 
     // do some things 
    } 
    finally 
    { 
     // do some cleanup 
    } 
} 
+0

Je vois l'erreur que j'ai faite - juste raté le dernier parce que ça ne rentrait pas sur mon écran - mais il ne sert à rien de réécrire le mien pour le faire ressembler au tien. – tvanfosson

+0

C'est ce que je ferais. Je ne pense pas que je partagerais ce morceau dans une autre fonction comme l'a fait un autre répondant, à moins que je ne m'attendais à utiliser cette logique à nouveau. – BobbyShaftoe

0

Ceci honore les conditions initiales sous lesquelles le fichier est supprimé et est plus propre et plus compact. Ma première réponse a défini folderId, mais j'ai alors réalisé qu'il n'était pas utilisé.

public static void HandleUploadedFile(string filename) { 
    try { 
     if(IsValidFileFormat(filename) 
     && GetFolderIDFromFilename(filename)>0 
     && HasNoViruses(filename) 
     && VerifyFileSize(filename)) { 
      MoveToSafeFolder(filename);         // file is OK 
      } 
     else { 
      DeleteFile(filename); 
      } 
     } 
    catch (Exception ex) { 
     // HANDLE THE EXCEPTION; AT LEAST LOG IT! 
     } 
    finally { 
     // do some things 
     } 
    } 
0

C'est ma solution à factoriser votre cas (désolé pour le style java):

Je vais créer une classe UploadedFileChecker mis en œuvre comme suit:

private class UploadedFileChecker { 
    String fileName; 

    UploadedFileChecker(String fileName) { 
     this.fileName = fileName; 
    } 

    public void check() throws BadUploadedFileException { 
     checkNameFormat(); 
     checkFolderId(); 
     scanVirus(); 
     checkFileSize(); 
    } 

    private void checkFolderId() { 
     // throw BadUploadedFileException if not passed. 
    } 

    private void checkNameFormat() { 
     // throw BadUploadedFileException if not passed. 
    } 

    private void scanVirus() { 
     // throw BadUploadedFileException if not passed. 
    } 

    private void checkFileSize() { 
     // throw BadUploadedFileException if not passed. 
    } 
} 

À l'exception

public class BadUploadedFileException extends RuntimeException { 

} 

Et le handleUploadedFile refactorisé deviendra:

public static void handleUploadedFile(string filename) { 

    try { 
     checkFile(fileName); 
    } catch (BadUploadedFileException ex) { 
     deleteFile(fileName); 
    } catch (Exception ex) { 

    } finally { 

    } 
} 

private static void checkFile(String fileName) { 
    new UploadedFileChecker(fileName).check(); 
} 

private static void deleteFile(String fileName) { 

} 
Questions connexes