Blog Arolla

Man versus Legacy : Gilded Rose (deuxième partie )

Dans le premier épisode nous avions expliqué le contexte du kata et proposé une approche permettant de travailler la réécriture du code en toute sécurité. Pour cela nous avions écrit des tests de caractérisation, jusqu’à l’obtention d’un golden master.

Nous avons toute latitude pour refactorer le code afin d’y voir plus clair. La moindre erreur sera en effet mise en évidence par notre golden master, fournissant un vrai filet de sécurité.

Cette seconde partie propose différentes techniques permettant de refactorer le code pour arriver à quelque chose de plus compréhensible. A partir de ça, nous pourrons donner une dimension orientée plus métier au code, fournir des tests également centrés sur le métier, et au final nous affranchir du golden master.

La phase de refactoring

En entrant dans la phase de réécriture, il faut s’astreindre à nouveau à ces principes du TDD (Test-Driven Development):

  • Avoir un feedback quasi-permanent (ce qu’on obtient en ayant le réflexe de relancer les tests à chaque modification du code, ou encore en s’aidant d’un lanceur de test automatique)

  • Travailler par petits incréments (baby steps) et ne s’autoriser à poursuivre que tant qu’on est dans le vert

La conséquence principale est qu’en cas d’erreur, nous ne sommes qu’à un Ctrl+Z (Undo) de revenir à un état stable.

Lancer les tests c’est fastidieux …

Sérieusement ? C’est aussi pénible que ça ? On a presque l’impression d’entendre un ado se plaindre …

Relancer les tests, c’est juste l’affaire d’un raccourci clavier (Ctrl + F5 avec IntelliJ), et l’exécution est quasi-instantanée, à moins de développer avec une épave. Ce n’est donc pas la mer à boire.

Ceci étant dit, quand on est développeur, toutes techniques, astuces ou outils permettant de se faciliter la vie, même d’un epsilon, sont bons à prendre. La bonne nouvelle, c’est que ces outils existent : on parle de lanceurs automatiques de tests (comme NTests, Infinitests, ou directement intégrés à l’IDE). Ils permettent de rejouer automatiquement un ensemble sélectionné de tests à la moindre modification du code (sous réserve qu’il compile toujours). On peut alors se concentrer sur le refactoring pur et dur, on sait qu’on recevra (en principe) du feedback dès que la modification de code entraîne une modification de comportement.

Même si ces lanceurs automatiques sont pratiques, ils peuvent se révéler capricieux : ils peuvent s’arrêter de fonctionner correctement du jour au lendemain, et la configuration initiale peut être une source de frustration. De plus, ils se prêtent assez mal à des volumes de code importants (pour des raisons de performance) et sont donc à réserver pour des interventions ciblées.

Le plus important est que les tests soient exécutés à chaque modification du code, que ce soit sous la forme d’un réflexe, ou d’une façon automatique. Notons que bénéficier d’un lanceur automatique, même si c’est bien pratique, ne nous aidera pas des masses à acquérir ce réflexe.

Par où commencer ?

Dans le cas de Gilded Rose, le démarrage du refactoring ne saute pas vraiment aux yeux.

Le premier réflexe est de réduire d’emblée la duplication. Pour cela, on recherche des morceaux de code récurrents, qui semblent être de bons candidats pour effectuer, selon le contexte des Extract Constant, Extract Variable ou Extract Method. En fournissant des noms judicieux, on a même l’impression de fournir un code mieux documenté.

Habituellement, les actions de refactoring suivantes sont observées :

  • extraire des constantes pour les noms d’articles

  • extraire des méthodes pour les conditions sur les noms : isSulfuras(), isAgedBrie() voire des choses comme isAgedBrieOrBackstage() : ce genre d’abstraction est dangereux parce qu’il est susceptible de masquer une duplication évidente sur le type d’article Aged Brie

  • extraire des méthodes pour incrémenter ou décrémenter la qualité

Hé ! Pas si vite ...

En procédant ainsi, on avance pendant un temps relativement bref, puis arrive le moment où on se retrouve coincé. On a beau scruter le code, on ne voit pas comment continuer. Toute tentative de refactoring supplémentaire réduit davantage le champ des possibles.

Voici quelques raisons qui font qu’on en arrive là :

  • La complexité cyclomatique (i.e. le nombre de niveaux de branchement) reste importante

  • On a du mal à identifier des niveaux d’abstraction, tout est mélangé; une même considération (par exemple les conditions sur le nom de l’article) se retrouve présente à différents niveaux de branchement

  • Les attributs quality et sellIn sont utilisés en lecture (dans des conditions) et en écriture (incréments, décréments); modifier le code associé peut entraîner des effets de bord imprévus

  • On trouve parfois de la fausse duplication (conditions paraissant semblables mais qui sont en fait inversées)

  • Les lignes dupliquées ne se situent pas toujours au même niveau de branchement

Le leurre de l’optimum local

Si on est bloqué, c’est parce qu’on s’est retrouvé dans une situation d'optimum local. Pour s’en sortir, il faut accepter de dégrader du code de façon temporaire. En se retrouvant dans une situation (en apparence) moins favorable, on va pouvoir l’améliorer ensuite. C’est comme en alpinisme, pour atteindre un sommet plus haut, il est parfois nécessaire de redescendre un peu pour trouver une voie plus praticable. Ou comme lorsqu’on résout un Rubik’s Cube, en acceptant de défaire temporairement une face complète.

En fait, on doit dès le départ faire quelque chose de totalement contre-intuitif : introduire encore plus de duplication. Même si le code initial s’étale sur de nombreuses lignes de code, il ne faut pas avoir peur de le rendre encore plus volumineux.

Vu sous un autre angle, on fait un peu comme en maths, où il est parfois nécessaire de développer certaines sous-expressions si on veut aboutir à une factorisation globale satisfaisante.

Les approches de refactoring décrites reposent sur ce principe de départ.

Une première étape facile

Pour se donner du coeur à l’ouvrage et ne pas rester bloqué, nous allons commencer par un premier Extract Method au niveau global. Il s’agit d’extraire l’intérieur de la boucle dans une méthode dédiée. On pourrait le faire à la main, au risque de se louper, mais avec un IDE évolué (IntelliJ par exemple), il suffit de suivre ces étapes :

  • on sélectionne tout le code à l’intérieur de la boucle

  • on effectue l’action de refactoring Extract Method

  • IntelliJ nous propose de créer une méthode dont la signature est void updateQuality(Item item)

  • on valide et c’est fait (et on n’oublie pas de s’assurer que les tests sont toujours OK)

Maintenant, on peut prendre un instant pour admirer l’intelligence de l’IDE : la méthode créée aurait put avoir un paramètre de type int, correspondant au compteur de boucle. Seulement, IntelliJ a détecté que le seul usage du compteur de boucle i se situe dans l’expression suivante :

items[i]

dont le type est Item. D’où le fait de proposer directement un paramètre de ce type.

Remarque : avec un IDE moins futé, on procéderait en 2 étapes. Il suffirait de remplacer la boucle avec index par une boucle for each sur le type Item, et ensuite d’extraire la méthode.

On y gagne en élégance, en lisibilité mais aussi en qualité du design. La méthode updateQuality(Item item) n’a pas besoin de connaître l’état interne de la classe GildedRose (ce qui aurait été le cas en reprenant le compteur de boucle en paramètre), ce qui est préférable en termes d’encapsulation et de découplage. Il serait alors possible de déplacer cette méthode dans un composant dédié.

Approche classique

Nous nous basons sur le refactoring effectué par David Gageot dans la session suivante : Du Legacy au Cloud en moins d'une heure. Le challenge est de taille puisque le speaker doit décrire le problème, écrire les tests, refactorer le code, construire une application Web simple et déployer sur le Cloud, tout cela en un temps très court.

Ces contraintes font que même si l’ensemble est brillamment exécuté, le performer présente un contenu très dense et énormément de techniques, de façon peut-être trop rapide. C’est pourquoi nous faisons le choix de nous attarder sur les différentes étapes.

Nous avons vu qu’il était inapproprié de chercher immédiatement à réduire la duplication, sous peine de se retrouver coincé. Dans le cas de Gilded Rose, même si c’est contre-intuitif, il ne faut pas avoir peur de faire grossir le code en augmentant ponctuellement la duplication.

Etapes du refactoring

Simplifier les conditions (c’est pas faux !)

On va chercher à transformer les conditions complexes en un ensemble de conditions plus simples. L’idée est de supprimer les NOT, AND ou OR dans l’optique d’obtenir uniquement des conditions à un seul terme.

Ce faisant, on va rendre le code en apparence plus complexe et volumineux, mais on obtient une meilleure vision de la logique de branchement. De plus, on va faire émerger de la duplication entre certaines conditions, offrant ainsi des opportunités de nettoyage du code. En fait, on cherche à augmenter la duplication pour pouvoir la tuer.

Voici les patterns de réécriture qui s’appliquent :

Type de condition

Avant réécriture

Après réécriture

NOT

if (!cond) {
   // ... bloc1
} else {
   // ... bloc 2
}
if (cond) {
   // ... bloc2
} else {
   // ... bloc 1
}

AND

if (condA && condB) {
   // ... bloc
}
if (condB) {
   if (condA) {
       // ... bloc
   }
}

OR

if (condA || condB) {
   // ... bloc
}
if (condA) {
   // ... bloc
} else if (condB) {
   // ... bloc
}

Cette étape demande assez peu d’efforts, l’IDE s’occupe de tout. Dans le cas d’expressions plus complexes mélangeant plusieurs opérateurs, on se basera sur la priorité des opérateurs. Dans le cas de Gilded Rose, on supprime d’abord les conditions NOT. Il ne reste alors qu’une condition OR à réécrire.

Cette étape produit les effets suivants :

  • certaines sous-conditions deviennent mutuellement exclusives avec une condition de plus haut niveau, indiquant un code inatteignable (ou mort). On peut alors supprimer la sous-condition et le bloc d’instructions qu’elle contient

  • d’autres sous-conditions deviennent redondantes (parce que identiques) avec une condition de plus haut niveau. On peut alors supprimer la sous-condition, mais pas le bloc de code qu’elle contient. Il faut néanmoins veiller à l’absence d’effet de bord entre les deux conditions pour faire la suppression sans risques

Un rappel important : en cas de doute, ne pas oublier que la présence du harnais de test, associé au fait de lancer les tests à chaque modification du code, offrent des garanties suffisantes.

Le cas Sulfuras

Nous commençons à avoir l’intuition que le nom des articles est au coeur de la logique métier :

  • les valeurs possibles appartiennent à un espace discret très petit, ce qui offre une forme de catégorisation

  • l’évolution demandée (ajouter une règle de gestion sur les Conjured Items) nous oriente naturellement sur ce choix

  • un certain nombre de conditions basées sur le nom apparaissent au premier niveau de branchement

On va donc chercher progressivement à placer les conditions propres au nom au niveau le plus externe.

On constate que le type Sulfuras est particulier : le succès de la condition associée ouvre systématiquement sur un bloc de code vide. Nous faisons alors le choix (payant) de remonter le cas Sulfuras dans une unique condition de garde en début de méthode :

if (item.name.equals("Sulfuras, Hand of Ragnaros")) {
   return;
}

Une symétrie commence à apparaître dans le code sur le nom des articles, ce qui permet d’entrevoir de façon subtile et prometteuse le métier.

Des conditions presque identiques

Le bloc de code suivant apparaît à de nombreuses reprises :

if (item.quality < 50) {
   item.quality = item.quality + 1;
}

C’est donc un candidat idéal pour un Extract Method. Seulement, il reste un endroit avec la configuration suivante :

if (item.quality < 50) {
   item.quality = item.quality + 1;
   // ... autres instructions
}

C’est presque le même code, mais les instructions à la suite nous embêtent. En examinant le code plus en détails, on a quand même l’intuition qu’on pourrait remonter l’accolade fermante avant les autres instructions sans que ça ne pose de problèmes.

Heureusement, notre filet de sécurité nous permet de valider cette hypothèse et de procéder à l’extraction d’une méthode increaseQuality().

De façon symétrique nous extrayons également decreaseQuality() :

private void decreaseQuality() {
   if (item.quality > 0) {
       item.quality = item.quality - 1;
   }
}

Briser les règles pour mieux voir le code

On cherche maintenant à visualiser l’ensemble du code sur un seul écran. Pour cela, nous choisissons de renier les conventions de code et de supprimer les accolades pour les conditions sur le sellIn. On obtient alors un code plus compact et on voit apparaître des structures.

La Muraille de Chine

Cette portion du code nous est particulièrement pénible :

item.sellIn = item.sellIn - 1;
if (item.sellIn < 0) {

Elle agit comme une barrière au milieu du code, une sorte de Muraille de Chine qui nous empêche de fusionner et d’avoir au même niveau les blocs de code basés sur les noms d’articles.

Une idée pour s’en sortir ? Une fois encore, en augmentant temporairement la duplication. Mais pas n’importe comment :

  • l’instruction d’incrément du sellIn va être dupliquée dans chacun des blocs du haut

    • attention à sa position dans le bloc sur le Backstage, il faut le laisser après les conditions sur le sellIn pour ne pas provoquer de régressions

  • la condition sur le sellIn va être dupliquée dans chacun des blocs du bas

Nos conditions liées aux noms d’articles se retrouvent alors toutes au même niveau, le plus externe, sans séparation entre elle. On peut donc les fusionner facilement.

Décrément du sellIn

Malgré ça, le décrément du sellIn continue à nous ennuyer : cette ligne de code apparaît 3 fois. On aimerait bien la remonter avant la condition sur le Aged Brie. Seulement, le comportement du Backstage nous en empêche.

On part de ça :

if (item.sellIn < 11) increaseQuality(item);
if (item.sellIn < 6) increaseQuality(item);
item.sellIn = item.sellIn - 1;

En faisant ça, les tests échouent :

item.sellIn = item.sellIn - 1;
if (item.sellIn < 11) increaseQuality(item); // tests KO
if (item.sellIn < 6) increaseQuality(item);

On résout le problème en rétablissant l’équilibre dans les conditions :

item.sellIn = item.sellIn - 1;
if (item.sellIn + 1 < 11) increaseQuality(item);
if (item.sellIn + 1 < 6) increaseQuality(item);

Ce qui se simplifie en :

item.sellIn = item.sellIn - 1;
if (item.sellIn < 10) increaseQuality(item);
if (item.sellIn < 5) increaseQuality(item);

Ce qui ne pose plus aucun problème pour remplacer les 3 décréments en un seul avant la condition sur le Aged Brie. Cette duplication introduite temporairement est ainsi éliminée.

Les touches finales

Nous sommes maintenant libres d’effectuer au choix les améliorations suivantes :

  • déléguer la mise à jour des articles à une classe ItemUpdater

  • remplacer les if … else par un switch

  • mieux encore, mettre en oeuvre un pattern Strategy basé sur le nom des articles

Mais c’est vraiment si on dispose d’encore un peu de temps. L’important est que nous soyons arrivé à un code beaucoup plus lisible, qui permet de bien comprendre le métier sous-jacent. Nous avons ainsi l’opportunité d’implémenter facilement et sans risques  l’évolution demandée.

GitHub : https://github.com/athiefaine/gilded-rose-kata/tree/classic_way (chacune des étapes correspond à un commit spécifique)

Approche ludique

Woody Zuill propose une approche de refactoring beaucoup plus originale mais diablement efficace.

Encore plus de duplication !

Elle se base également sur l’intuition qu’une bonne partie de la logique métier se base sur le nom des articles, agissant comme une catégorisation. On va alors chercher à construire un chemin spécifique pour chaque catégorie d’article, isolant ainsi la logique métier qui lui est propre.

Dans le code initial, chacune de ces logiques métier est entremêlée avec les autres. L’idée est d’isoler chacune de ces logiques, l’une après l’autre. En choisissant une première catégorie d’article, nous créons deux chemins dans le code :

  • le premier pour lequel l’article est dans la catégorie choisie

  • le second pour lequel l’article appartient à n’importe quelle autre catégorie

Commençons par le Aged Brie :

if (item.name.equals("Aged Brie")) {
} else {
}
// ... legacy code

Le problème consiste maintenant à fournir une implémentation pour chacune des deux branches. Ca paraît difficile, étant donné qu’on ne comprend pas vraiment la logique métier ...

Le fait est que nous avons une implémentation qui fonctionne pour les deux branches, juste sous nos yeux : le code legacy en lui-même !

Réécrivons le code en introduisant temporairement davantage de duplication :

if (item.name.equals("Aged Brie")) {
// ... legacy code
} else {
    // ... duplicated legacy code
}

Les tests passent toujours, c’est incroyable ! Mais nous ne sommes pas plus avancés ...

Pourtant, si nous avons introduit de la duplication (le volume de code a littéralement été multiplié par 2), c’est pour mieux la tuer par la suite. La question est d’identifier les portions de code à supprimer …

La couverture de code comme outil de refactoring

Dans la première partie de l’article, nous nous étions appuyés sur un outil de couverture de code pour déterminer la robustesse et la pertinence de notre harnais de tests. Cet objectif atteint avec une couverture optimale, on était en droit de supposer que la couverture de code ne nous était d’aucune utilité pour la phase de refactoring. En fait, c’est tout l’inverse, la couverture de code va nous fournir une aide précieuse.

Relançons les tests avec la couverture de code activée, et observons ce qu’il se passe pour la branche Aged Brie :

Les zones en rouge désignent le code qui n’a pas été exécuté par les tests. Cela signifie que ces lignes de code n’ont aucune utilité pour que notre nouvelle implémentation soit iso-fonctionnelle avec l’ancienne.

En d’autres termes, c’est du code mort, et en tant que tel il peut être supprimé sans danger.

En combinant la couverture de code et les indications de l’IDE, on peut appliquer les patterns suivants en vue de simplifier le code sans risques :

  • ligne dont la couverture est rouge : c’est du code mort qui peut être supprimé

  • condition signalée comme toujours fausse (par exemple elle est mutuellement exclusive avec une condition à un plus haut niveau) : on peut enlever la condition et le bloc qu’elle contient

  • condition signalée comme toujours vraie (cela arrive si elle est redondante avec une condition de plus haut niveau) : on procède à un unwrap (on “déballe” le bloc de code, i.e. on le conserve mais on supprime la condition qui l’englobe)

  • un if ou un else vide : on supprime

Une fois toutes ces opérations effectués, le chemin pour Aged Brie ressemble à ça :

 

Le fait qu’on retrouve une couverture optimale indique qu’on peut difficilement faire mieux à ce stade.

Le niveau de simplification est assez impressionnant. Mais ce qui l’est encore plus, c’est d’être arrivé à ce résultat sans se faire de noeuds au cerveau. En effet, nous nous sommes laissés guider par nos outils et les automatismes de l’IDE, en ne prenant aucun risque.

De plus, c’est assez ludique, on cherche juste à tuer les lignes qui ne sont pas couvertes. Cette façon de faire me fait penser à certains jeux de réflexion qu’on pratique en mode semi-automatique, comme les match-3, le 2048 ou encore Tetris. Je vous invite donc vivement à refaire ce refactoring à la maison pour en expérimenter le côté plaisant.

Rinse and repeat

L’idée est ensuite de passer aux catégories suivantes : Backstage, Sulfuras, avant de terminer par la catégorie d’article par défaut.

GitHub : https://github.com/athiefaine/gilded-rose-kata/tree/playful_way (chacune des étapes correspond à un commit spécifique)

Variante

Dans cette approche, nous n’avons supprimé le code mort que pour la branche “vraie” de la condition.

Une variante consiste à éliminer le code mort pour les deux branches de la condition à chaque tour. La branche concernant les autres cas se retrouve alors simplifiée de façon plus progressive.

Adopter l’une ou l’autre des variantes est une affaire de goût, on aboutit au même résultat final.

GitHub : https://github.com/athiefaine/gilded-rose-kata/tree/playful_way_alt (chacune des étapes correspond à un commit spécifique)

La couverture de code, un couteau suisse ?

Ou “pourquoi ça fonctionne” ?

La couverture de code sert au départ à identifier les lignes de code qui ne sont pas testées. Lorsqu’on atteint une couverture optimale (proche de 100%), on peut être rassuré sur le fait que le code est suffisamment testé.

C’est le cas pour notre code legacy, aucun doute là-dessus. Le fait qu’un code alternatif fournissant exactement les mêmes fonctionnalités comporte des lignes non couvertes leur confère une nature de code mort.

Bien sûr, cette propriété provient de la nature de nos tests, en golden master, où on compare le comportement de deux implémentations différentes. Ce contexte précis entraîne que la couverture de code peut être employée comme détecteur de code mort, ce qui constitue le point-clé de cette approche.

Point de convergence

A ce stade, nous atteignons un code relativement lisible, mais pas aussi épuré qu’avec l’approche classique. Nous pouvons appliquer certaines de ses étapes pour améliorer le code :

  • extraire increaseQuality() et decreaseQuality()

  • remonter le décrément sur le sellIn pour en réduire la duplication

On s’épargne les difficultés suivantes de l’approche classique :

  • ne pas s’embêter à simplifier les conditions

  • ne pas être confronté à la Muraille de Chine sur le sellIn et obtenir des conditions sur les noms fusionnées dès le départ

L’inconvénient majeur est que l’approche ludique ainsi que son hypothèse de départ (catégoriser sur les noms d’articles) ne sont pas nécessairement intuitives.

Passons aux choses sérieuses

Une fois le refactoring effectué, on obtient un code plus lisible, plus compréhensible. On commence à avoir une vision plus claire du métier.

On pourrait s’estimer satisfait et être tenté de s’arrêter là, d’autant que le golden master fournit la couverture de test souhaitée.

Le problème réside dans le fait que cela nous obligerait à conserver à la fois:

  • Un code de test qui n’apporte aucune documentation sur la fonctionnalité, il se contente juste de comparer le comportement du code legacy et du code refactoré

  • Un code legacy bien pourri (raison d’être de ce refactoring) qui va polluer la base de code à vie

De plus, toute évolution devrait être rétro-implémentée dans le code legacy (ce qui n’est pas une mince affaire) pour garantir le bon fonctionnement des tests.  Et nous avons une règle supplémentaire à produire, sur les articles de type Conjured. Conserver le golden master nous empêche de faire évoluer le code.

Il serait plus raisonnable de commencer à isoler des règles métiers, ces dernières pouvant être explicitées dans des tests dédiés. On peut par exemple dédier des tests à chaque type d’article, vérifiant la qualité et le nombre de jours avant la vente. L’objectif est d’obtenir également une couverture de 100% avec ces tests, en excluant bien sûr le golden master des mesures. Une fois ceci atteint, le golden master peut définitivement être mis à la poubelle.

Un effet secondaire (et positif) de l’écriture de ces tests orientés métier réside dans le fait qu’on achète de la connaissance supplémentaire sur le fonctionnel. Dès lors, on peut apporter des refactoring supplémentaires guidés par cette compréhension accrue du métier.

Testons le métier

Avant de commencer, déplaçons l’implémentation legacy dans les sources de test. Ceci évitera qu’elle soit utilisée par erreur dans le code de production. Notons qu’au passage, on perd la couverture sur ce code legacy, mais ce n’est pas très grave, l’idée étant de se débarrasser du golden master.

Nous allons maintenant écrire des tests basés sur notre compréhension métier. L’objectif est d’obtenir un ensemble de tests qui explicitent les règles de gestion, tout en atteignant une couverture de code optimale.

L’idée est de commencer par les règles paraissant les plus simples, puis de monter progressivement en complexité.

Sulfuras l’imputrescible

L’article Sulfuras est un bon candidat : quoiqu’il arrive, ses propriétés ne changent pas. Nous créons une classe de test pour le composant ItemUpdater, puis une méthode de test dédiée à Sulfuras. Dans celle ci, nous appelons updateQuality() un certain nombre de fois (16 fois, comme pour le golden master), et vérifions que les valeurs initiales de sellIn et de quality ne bougent jamais.

Au passage, lorsqu’on lance la couverture de tests, on constate que seule la condition de garde sur Sulfuras est couverte, ce qui était prévisible.

Le Vieux Brie : plus ça pue et meilleur c’est

Ce type d’article possède des propriétés odorantes et gustatives qui au final rappellent notre code legacy. Les tests émergent facilement de la lecture du code :

  • Un premier test vérifiant que la qualité s’améliore à chaque jour qui passe

  • Un second test pour s’assurer que la qualité s’améliore encore plus lorsque le sellIn est expiré

Place de concert : la hype monte !

La qualité du Backstage s’améliore de plus en plus vite au fur et à mesure que les jours passent, avec 2 paliers : à 10 jours, puis à 5 jours du terme. Attention toutefois, une fois la date du concert passée (sellIn expiré), l’article ne vaut plus un clou. Nous avons donc besoin des tests suivants :

  • La qualité augmente chaque jour à plus de 10 jours du terme

  • Elle augmente deux fois plus vite entre 5 et 10 jours du terme

  • Elle augmente trois fois plus vite entre 5 jours et le terme

  • Passé le terme, la qualité est nulle

Les articles communs

Ajoutons deux autres tests :

  • Par défaut, la qualité diminue quotidiennement

  • Par défaut, elle diminue deux fois plus vite une fois le sellIn expiré

Ajoutons également des tests vérifiant que la qualité est toujours comprise entre 0 et 50, et nous obtenons une couverture de 100%.

GitHub : https://github.com/athiefaine/gilded-rose-kata/tree/domain_tests

Mutation testing

Toutes les lignes de notre code sont couvertes, mais cela ne veut pas dire que chaque ligne de code est pertinente ni que les valeurs importantes sont couvertes. Pour s’en assurer, nous utilisons PIT, un outil de mutation testing. La mise en oeuvre est assez simple, nous lançons la tâche dédiée du build et allons consulter le rapport généré.

On se rend compte que certaines mutations ont survécu. Elles sont toutes du même type et liées aux bornes de conditions. L’outil a produit des mutations (en rouge ci-dessous) en remplaçant l’opérateur < par <=, et lance une alerte si les tests passent quand même.

 

En fournissant des valeurs de sellIn plus proches des bornes dans les tests, on arrive à tuer les mutations.

Avec une couverture de test à 100% accompagné d’un mutation testing pour lequel aucune mutation ne survit, on peut estimer que nos tests orientés métier sont très solides.

GitHub : https://github.com/athiefaine/gilded-rose-kata/tree/mutation_testing

Et l’évolution demandée, elle vient ?

En retrouvant à nouveau à une couverture de tests optimale, de nouvelles possibilités s’offrent à nous.

Il est temps de mettre le golden master (classe de test + implémentation legacy) à la poubelle, l’ensemble de tests orientés domaine étant suffisamment couvrant.

Et nous pouvons enfin réaliser ce pourquoi nous avons été sollicités, c’est-à-dire permettre de gérer une nouvelle classe d’articles, les Conjured. Leur qualité diminue deux fois plus vite que pour les articles classiques.

La meilleure façon de procéder est de suivre une approche en TDD (test-driven development), à savoir écrire les tests décrivant la nouvelle fonctionnalité, puis de faire fonctionner l’implémentation.

On commence par un premier test qui vérifie que la qualité diminue deux fois plus vite. Il échoue, on gère le cas pour faire fonctionner.

Un second test permettra de vérifier le cas où l’article est périmé (sellIn < 0), avec une qualité qui diminue alors de 4 points par jour.

Comme on travaille sur du code réécrit et relativement lisible, supporter cette nouvelle fonctionnalité est très facile. Voici une implémentation très naïve mais qui fonctionne :

if (item.name.equals("Conjured")) {
   decreaseQuality();
   decreaseQuality();
   if (item.sellIn < 0) {
       decreaseQuality();
       decreaseQuality();
   }
}

Il reste encore plusieurs possibilités d’amélioration du code, mais nous allons nous arrêter là, l’objectif d’implémenter de façon fiable l’évolution demandée avec un code lisible étant atteint.

Rétrospective

On retrouve sur cette seconde partie les enseignements suivants :

  • Il est important de prendre du recul et d’expérimenter des approches contre-intuitives, notamment en augmentant de façon temporaire la duplication

  • Il est crucial de s’appuyer sur les outils (IDE, couverture de code) pour éviter de se faire trop de noeuds au cerveau

  • La couverture de code peut fournir un faux sentiment de sécurité, il est important d’en comprendre les limites et le cas échéant de la compléter avec des approches complémentaires comme le mutation testing

Nous avons également découvert les aspects suivants :

  • Il n’est pas nécessaire de comprendre tout de suite ce que fait le code, et qu’en se basant sur des outils et automatismes la compréhension va émerger progressivement

  • Il faut lancer les tests à la moindre modification pour bénéficier d’un feedback permanent

  • On peut refactorer de plusieurs façons différentes, certaines permettant de vraiment s’amuser

  • Le golden master doit rester éphémère, c’est un échafaudage nécessaire au départ mais qui doit disparaître une fois le code rendu plus clair

  • Il est important de savoir s’arrêter, sans rechercher la perfection du code : le fait que le code soit compréhensible, permettant d’écrire des tests proches du domaine et de réaliser les évolutions demandées constitue un bon signal

Remerciements

Je tiens à remercier encore une fois les personnes suivantes pour le temps passé à relire mon texte, et les différents conseils, idées et encouragements qu’ils ont pu me prodiguer : Dorra Bartaguiz, Alexia Hoang, Benjamin Hugot, Arnauld Loyer, Cyrille Martraire, Laury Maurice, Hadrien Mens-Pellen, Nicolas Morin et Yvan Phélizot.

Plus de publications

2 comments for “Man versus Legacy : Gilded Rose (deuxième partie )

  1. Debove Christophe
    16 avril 2021 at 19 h 36 min

    Super article, merci beaucoup. L’approche est très pédagogique.
    Pourquoi abandonner le Golden master ça reste un bon harnais finalement?
    Pourrait-on pousser et écrire les spec en mode bdd avec gherkin?(3eme article…) et en extraire une documentation vivante.

  2. Arnaud Thiefaine
    17 avril 2021 at 9 h 12 min

    Merci Christophe pour ces retours.

    Le golden master permet de mettre en place un harnais de sécurité sans trop se poser de question, l’important est que le code soit couvert intégralement pour travailler en toute sécurité. Seulement, ces tests ne reflètent en aucune manière les règles métiers du code testé, et alors on passe à côté d’un aspect important des tests : fournir de la documentation. Néanmoins, comme en refactorant, on a gagné en compréhension sur le comportement, c’est le moment de transcrire ces connaissances sous forme de tests qui portent le métier. Ce n’est pas obligatoire, mais c’est préférable.

    Concernant l’aspect BDD : je pense que ça ne s’y prête pas dans tous les cas. De plus, le plus important dans l’approche BDD est de fournir l’opportunité d’avoir des conversations avec les experts du métier, Gherkin aidant à le faire au moyen d’un format compréhensible à tous. Dans le cadre d’un refactoring de code legacy, on arrive un peu après la bataille, donc l’investissement nécessaire pour automatiser du Gherkin ne sera peut-être pas assez rentable ; à moins que ce ne soit le départ pour mettre en place de nouvelles fonctionnalités en se basant sur l’approche BDD !