«
»

WTF : refactoring…

Qui n’est jamais tombé sur du code écrit par d’autres complètement illisible avec des conditions imbriquées à rallonge et totalement incompréhensibles ? Il faut souvent de longues minutes d’intense réflexion pour trouver la signification à tout ce charabia.

C’est mon cas aujourd’hui et je vous en fais partager. La classe en elle même (avec ses 2078 lignes avant refactoring) est elle-même un concentré de WTF, mais concentrons nous sur une toute petite méthode :

  1. public boolean isStatutLigneAnalyseConforme(List<LigneAnalyseVO> ligneAnalyseList) {
  2.  if (ligneAnalyseList != null && ligneAnalyseList.size() > 0) {
  3.   for (LigneAnalyseVO ligneAnalyse : ligneAnalyseList) {
  4.    if (ligneAnalyse.getStatutByStaIdPrescripteur() != null
  5.      && !ligneAnalyse.getStatutByStaIdPrescripteur().getStaCode()
  6.        .equals(StatutEnum.ANALYSE_USA_PRESCRIPTEUR_ENGAGE.getStaCode())
  7.      && (ligneAnalyse.getStatutByStaIdPrescripteur() != null
  8.        && !ligneAnalyse
  9.          .getStatutByStaIdPrescripteur()
  10.          .getStaCode()
  11.          .equals(StatutEnum.ANALYSE_USA_PRESCRIPTEUR_CLOTURE
  12.            .getStaCode()) && (ligneAnalyse
  13.        .getStatutByStaIdPrescripteur() != null && !ligneAnalyse
  14.        .getStatutByStaIdPrescripteur().getStaCode()
  15.        .equals(StatutEnum.ANALYSE_USA_PRESCRIPTEUR_TERMINE.getStaCode())))) {
  16.     return false;
  17.    }
  18.   }
  19.  }
  20.  return true;
  21. }

Etape 1 : extraction d’une variable locale

« ligneAnalyse.getStatutByStaIdPrescripteur() » c’est trop long à écrire et trop long à lire et puis faire plusieurs fois l’appel au même getter c’est pas une bonne pratique. Donc, on extrait une variable locale et on lui donne un nom explicite : « statutPrescripteur ».

  1. public boolean isStatutLigneAnalyseConforme(List<LigneAnalyseVO> ligneAnalyseList) {
  2.  if (ligneAnalyseList != null && ligneAnalyseList.size() > 0) {
  3.   for (LigneAnalyseVO ligneAnalyse : ligneAnalyseList) {
  4.    StatutVO statutPrescripteur = ligneAnalyse.getStatutByStaIdPrescripteur();
  5.    if (statutPrescripteur != null
  6.      && !statutPrescripteur.getStaCode().equals(
  7.        StatutEnum.ANALYSE_USA_PRESCRIPTEUR_ENGAGE.getStaCode())
  8.      && (statutPrescripteur != null
  9.        && !statutPrescripteur.getStaCode().equals(
  10.          StatutEnum.ANALYSE_USA_PRESCRIPTEUR_CLOTURE.getStaCode()) && (statutPrescripteur != null && !statutPrescripteur
  11.        .getStaCode().equals(
  12.          StatutEnum.ANALYSE_USA_PRESCRIPTEUR_TERMINE.getStaCode())))) {
  13.     return false;
  14.    }
  15.   }
  16.  }
  17.  return true;
  18. }

Etape 2 : suppression des tests en double

Le test statutPrescripteur != null est fait trois fois. Une fois pour chaque valeur possible. C’est trop. Donc pour ne pas faire d’erreur en refactorant, on extrait ce test dans un autre if.

  1. public boolean isStatutLigneAnalyseConforme(List<LigneAnalyseVO> ligneAnalyseList) {
  2.  if (ligneAnalyseList != null && ligneAnalyseList.size() > 0) {
  3.   for (LigneAnalyseVO ligneAnalyse : ligneAnalyseList) {
  4.    StatutVO statutPrescripteur = ligneAnalyse.getStatutByStaIdPrescripteur();
  5.    if (statutPrescripteur != null) {
  6.     if (!statutPrescripteur.getStaCode().equals(
  7.       StatutEnum.ANALYSE_USA_PRESCRIPTEUR_ENGAGE.getStaCode())
  8.       && !statutPrescripteur.getStaCode().equals(
  9.         StatutEnum.ANALYSE_USA_PRESCRIPTEUR_CLOTURE.getStaCode())
  10.       && !statutPrescripteur.getStaCode().equals(
  11.         StatutEnum.ANALYSE_USA_PRESCRIPTEUR_TERMINE.getStaCode())) {
  12.      return false;
  13.     }
  14.    }
  15.   }
  16.  }
  17.  return true;
  18. }

Etape 3 : extraction de variable locale et inversion des if pour éviter des NPE

On extrait une deuxième variable locale : staCode.
A ce niveau là, on voit qu’un NullPointerException peut survenir si staCode est null car on ne teste pas que la variable peut être nulle.
Deux solutions s’offrent à nous :

  • soit rajouter une condition de non nullité sur staCode
  • soit inverser les trois tests, car on est sur de la non nullité des valeurs de notre enum.

On va opter pour la deuxième solution.

  1. public boolean isStatutLigneAnalyseConforme(List<LigneAnalyseVO> ligneAnalyseList) {
  2.  if (ligneAnalyseList != null && ligneAnalyseList.size() > 0) {
  3.   for (LigneAnalyseVO ligneAnalyse : ligneAnalyseList) {
  4.    StatutVO statutPrescripteur = ligneAnalyse.getStatutByStaIdPrescripteur();
  5.    if (statutPrescripteur != null) {
  6.     String staCode = statutPrescripteur.getStaCode();
  7.     if (!StatutEnum.ANALYSE_USA_PRESCRIPTEUR_ENGAGE.getStaCode().equals(staCode)
  8.       && !StatutEnum.ANALYSE_USA_PRESCRIPTEUR_CLOTURE.getStaCode().equals(
  9.         staCode)
  10.       && !StatutEnum.ANALYSE_USA_PRESCRIPTEUR_TERMINE.getStaCode().equals(
  11.         staCode)) {
  12.      return false;
  13.     }
  14.    }
  15.   }
  16.  }
  17.  return true;
  18. }

Etape 4 : extraction des valeurs possibles dans une liste

Mais maintenant, on voit que la liste des valeurs possibles est diluée dans le code et difficilement lisible / maintenable / modifiable. Donc on va extraire cette liste dans une variable static.

Nous utilisons ici une classe utilitaire de notre projet ListUtil qui propose une méthode générique isIn(E val, List<E> pool) pour vérifier qu’une valeur appartient bien a un ensemble d’éléments.

  1. private static final String[] STATUTS_CONFORMES = {
  2.   StatutEnum.ANALYSE_USA_PRESCRIPTEUR_ENGAGE.getStaCode(),
  3.   StatutEnum.ANALYSE_USA_PRESCRIPTEUR_CLOTURE.getStaCode(),
  4.   StatutEnum.ANALYSE_USA_PRESCRIPTEUR_TERMINE.getStaCode() };
  5.  
  6. public boolean isStatutLigneAnalyseConforme(List<LigneAnalyseVO> ligneAnalyseList) {
  7.  if (ligneAnalyseList != null && ligneAnalyseList.size() > 0) {
  8.   for (LigneAnalyseVO ligneAnalyse : ligneAnalyseList) {
  9.    StatutVO statutPrescripteur = ligneAnalyse.getStatutByStaIdPrescripteur();
  10.    if (statutPrescripteur != null) {
  11.     String staCode = statutPrescripteur.getStaCode();
  12.     if (!ListUtil.isIn(staCode, STATUTS_CONFORMES)) {
  13.      return false;
  14.     }
  15.    }
  16.   }
  17.  }
  18.  return true;
  19. }

Etape finale

N’est ce pas un peu plus lisible ?

Bon, la méthode est déclarée public, ne peut-on pas réduire sa visibilité ? Comme la classe est codée comme les pieds de trop nombreuses méthodes sont publiques alors qu’elles pourraient être private. Donc je vérifie quand même que la méthode est bien utilisée (reference / workspace) et là… oups, elle ne l’est pas : 0 reference found !

Donc étape ultime : suppression de la méthode !

Conclusion

Merci de penser aux autres quand vous codez, donc refactorez, refactorez et refactorez encore pour simplifier votre code. Ca sera utile aux autres et même à vous quand vous aurez à revenir dessus dans une semaine, un mois, un an.

Have fun.

Cet article a été publié le Mardi 19 juillet 2011 à 8 h 36 min et est classé dans Non classé. Vous pouvez en suivre les commentaires par le biais du flux RSS 2.0. Les commentaires et pings sont fermés.

Le commentaires sont fermés.