Comment cet extrait de code d'instruction IF peut-il être Refactorisé?

Question

J'ai lu beaucoup de littérature refactoring à la fin et un thème commun que je vois dans les exemples est la réduction de beaucoup de déclarations IF. Le ci-dessous est un bloc de code très simple qui vérifie d'abord si une adresse e-mail est fournie et si oui, puis procède à la validation de l'adresse e-mail. Je vois généralement beaucoup de ce type de code qui semble toujours un peu brouillon, surtout s'il y a plusieurs variables.

if (string.IsNullOrEmpty(email)) { 

    throw new ApplicationException("Email Address Required"); 
} 

else { 

    if (!ValidationService.EmailAddressIsValid(email)) { 
     throw new ApplicationException("Invalid Email Address"); 
    } 

} 

Ma question est, cet exemple est-il parfaitement acceptable? Est-ce que ça sent? Comment pourrait-on refactoriser cet extrait?

Answer 1

Il est difficile de factoriser un petit extrait - et je vais sauter la déclaration habituelle de ne pas refactoring sans essais ...

Quand je vois le code comme ça, je pense que

• Pourquoi le ValidationService ne contient-il pas la logique pour la vérification null? Si ce n'est pas dans une classe qui contient la logique de validation, alors pourquoi vérifions-nous les champs requis ici?

• je ne serais probablement faire le chèque nul en aide afin que le code pourrait être plus comme:

  ThrowIfRequiredFieldMissing(email, "Email Address");

• enlever l'autre, car un jet ne peut pas continuer

• Créer une aide pour le lancer: FailIf(!Validation.EmailAdressIsValid(email), "Email Address"). Cela jetterait votre exception de validation, et vous permettent de modifier des exceptions plus tard si nécessaire (bien que je traverse ce pont quand vous venez à lui)

Il y a beaucoup d'autres possibilités, comme un dictionnaire de Func<> s, ou un schéma de stratégie appliqué à une liste de champs, et ainsi de suite. Mais sans plus de code, décider quoi faire (si quelque chose!) Est juste deviner.

Answer 2

jet ne retourne jamais, donc j'utiliser ce qui suit pour une meilleure lisibilité:

if (string.IsNullOrEmpty(email)) { 
    throw new ApplicationException("Email Address Required"); 
} 

if (ValidationService.EmailAddressIsValid(email)) { 
    throw new ApplicationException("Invalid Email Address"); 
} 

// continue here if no error was detected 

Answer 3

Il est très bien, mais vous pouvez simplifier plus.

if (string.IsNullOrEmpty(email)) { 
    throw new ApplicationException("Email Address Required"); 
} else if (ValidationService.EmailAddressIsValid(email)) { 
throw new ApplicationException("Invalid Email Address"); 
} 

Answer 4

La première chose que vous pourriez faire serait de supprimer les lignes else { et son } correspondant. Ceci est dû au fait que la première instruction throw garantirait que le code actuellement dans le bloc else ne s'exécuterait jamais si l'adresse e-mail était manquante.

Answer 5

Le refactoring est plus qu'un simple style de code.

Vous validez une entrée d'utilisateur, vous souhaiterez probablement définir ou réutiliser un cadre de validation existant au lieu d'incorporer des instructions if dans le code. Vous êtes presque là, il semble, puisque vous avez le ValidationService pour la deuxième validation.

Donc ma proposition: Inclure le string.IsNullOrEmpty vérifier dans l'implémentation de EmailAddressIsValid.

Bien sûr, ce seul déplace une instruction if, mais si vous utilisez le EmailAddressIsValid à plusieurs endroits, cela réduira les instructions if. De plus, la logique de validation est alors réellement consolidée en un seul endroit. Actuellement ce n'est pas le cas.

Autre chose secondaire: Renommez EmailAddressIsValid en IsEmailAddressValid, car il est courant d'utiliser "Is" comme préfixe pour les fonctions renvoyant un booléen.

Answer 6

variante laid, mais réalisable

string msg = string.Empty;  
if (string.IsNullOrEmpty(email)) 
{ 
    msg = "Email Address required"; 
} 
else if (ValidationService.EmailAddressIsValid(email)) 
{ 
    msg = "Invalid Email Address"; 
} 

if (msg != string.Empty()) 
    throw new ApplicationException(msg); 

Answer 7

Si vous vraiment vous rendre votre code lisible, vous pouvez même faire quelque chose comme ceci:

throwExceptionIfEmailIsEmpty(email); 
throwExceptionIfEmailIsInvalid(email); 
// continue to process the e-mail here... 

Avec ce code extrait dans séparé fonctions:

private void throwExceptionIfEmailIsEmpty(String email) 
{ 
    if (string.IsNullOrEmpty(email)) 
     throw new ApplicationException("Email Address Required"); 
} 

private void throwExceptionIfEmailIsInvalid(String email) 
{ 
    if (ValidationService.EmailAddressIsValid(email)) 
     throw new ApplicationException("Invalid Email Address"); 
} 

Il pourrait sembler un peu excessif, mais cela rend beaucoup plus facile de voir ce qui se passe vraiment!
(Inspiré par Clean Code de l'oncle Bob - fonctions devraient faire une chose Ils devraient le faire bien ils devraient le faire que (ou quelque chose comme ça) - si quelqu'un se demandait,...))