Accueil Nos publications Blog Clean Code avec le Kata Video Store » Refacto d’oncle Bob (suite)

Clean Code avec le Kata Video Store » Refacto d’oncle Bob (suite)

Sherlock-Bob

Nous avons laissé oncle Bob à la fin du refacto des tests de notre video store. Déjà 20 minutes intenses ! Notre souffle repris, replongeons dans l’analyse de ce refacto, cette fois celui du code de production. Nommage, commentaires, fonctions courtes, on nagera avec bonheur en plein clean code. Les problématiques de refacto ne sont pas en reste, en nous attaquant à ce switch si dérangeant d’une perspective orientée-objet. Cela nous permettra d’utiliser à plein régime notre IDE.

Notre analyse se fera de manière thématique, successivement sur chacun de ces principes, plutôt que chronologique en suivant les refactos d’oncle Bob. Pour les retrouver, reportez-vous aux commits sur cette branche.

🔖 Sommaire : Nommage/Commentaires/Fonctions courtes/Organisation des fonctions/Refacto du switch·⏳ 20 min

Nommage

Lors du refacto des tests, oncle Bob a déjà commencé à améliorer le code de production en procédant à quelques renommages, au moyen du refactoring Rename qui marche un peu partout (namespace, type, méthode, champ, propriété, paramètre, variable locale, event et delegate) :

  • Voyant que la classe Customer concerne bien plus le rapport que le client, y figurant juste par son nom, il la renomme en Statement.
  • Du coup, le champ name devient customerName et la méthode statement() est renommée en generate(), ce qui permet de retrouver un verbe, en général plus adapté pour nommer une méthode.

Autres exemples de renommages :

  • Dans la boucle for (Rental each : rentals) {...}, la variable each a un nom technique, pour signifier que c’est cet élément qui change à chaque itération dans la boucle. Mais cette information n’est pas utile. Peu importe que l’on soit dans une boucle. Un nom “métier” aura plus de sens : tout simplement rental. S’il s’agissait d’une lambda, un nom plus court comme r ou x suffirait de par la portée plus courte : rentals.forEach(x => ...).
  • Dans les noms tels que thisAmount et myAmount, this et my n’ont aucune signification fonctionnelle. C’est du bruit pour rien. amount suffit, rentalAmount si l’on veut être très précis (choix opté par oncle Bob, même si personnellement je ne trouve pas que ce soit nécessaire).
  • De même, initialize n’est finalement pas assez précis pour oncle Bob qui opte pour clearTotals.

👉 Préconisation
N’utilisez pas _ comme nom générique pour un paramètre utilisé dans une lambda, ce qui donnerait dans notre exemple précédent rentals.forEach(_ => ...). Il est plus conventionnel de réserver ce nom à un paramètre inutilisé. De plus, je trouve que les expressions comme _.method() sont plus difficiles à lire. Enfin, que faire s’il y a un deuxième paramètre : le nommer __ ? On voit les limites de cette stratégie. Ce n’est pas le cas de x qui est suivi naturellement par y et z, formant un tout plus homogène.

Dans la suite du refacto, oncle Bob extrait des méthodes et les nomme sans verbe : header(), footer() plutôt que getHeader() ou computeFooter() par exemple. J’aime bien car cela se lit plus naturellement. Par contre, cela me semble approprié uniquement dans la mesure où le traitement effectué à l’intérieur de ces méthodes est simple et rapide. Dans le cas contraire, il vaut mieux prévenir l’utilisateur de la méthode qu’elle fait un gros calcul (computeXxx()) ou un appel distant (fetchXxx()) et donc en utilisant le verbe approprié à l’action à mettre en avant.

☝️ Notes
– Dans la version sur github oncle Bob utilise le verbe make : makeHeader(), makeSummary(). Comme quoi, le nommage est parfois une question d’humeur et d’envie 😆 !
– Cela n’empêche pas de respecter une certaine homogénéité dans le code. Si l’équipe de développement est partie sur une option de nommage, cela devient une norme de facto pour chaque membre de l’équipe. Par exemple, on peut choisir d’associer les Factories avec le verbe create. Alors, on n’utilisera pas d’autres termes même synonymes tels que make.

Un autre critère pour juger du nom d’une méthode : lire le code et vérifier que cela se fait naturellement, comme de l’anglais. C’est pour cette même raison que l’on évite les abréviations et les acronymes qui ne sont pas d’usage à l’oral.

Pour conclure sur le nommage, on voit Oncle Bob hésiter sur comment nommer. C’est normal : le nommage est une pratique délicate et difficile, y compris pour quelqu’un d’aussi expérimenté et talentueux qu’oncle Bob et alors même qu’il a déjà fait ce kata plusieurs fois. Jeff Atwood, auteur du blog Coding Horror et cofondateur de Stack Overflow, le formule ainsi dans un tweet de 2014 :

There are two hard things in computer science: cache invalidation, naming things, and off-by-one errors.

Voici matière à creuser le sujet :

Commentaires

En clean code, les commentaires sont considérés avec suspicion quand on en lit et sont à écrire avec parcimonie et connaissance de cause. On le comprend facilement avec des commentaires dans un registre émotionnel, exprimant fierté, humour, plainte… Cela semble surprenant au premier abord vu que les commentaires sont écrits pour aider à comprendre du code.

Exception faite du code mort, à supprimer de toute façon, tout code faux peut se constater car il est exécuté et génère un comportement inattendu. Ce n’est pas le cas des commentaires faux. Ils peuvent se révéler imprécis dès le départ, ne pas avoir été adaptés avec le code qui a évolué entre temps et auquel ils sont rattachés, ne plus être rattachés au code cible qui a été déplacés. Ils moisissent plus rapidement que le code. On dit que les commentaires sont des menteurs.

Quand bien même un commentaire serait juste, cela représente un surcoût de maintenance. Lorsque l’on suit les principes clean code, le code est dans la majorité des cas suffisamment compréhensible, concis, expressif et structuré pour ne pas nécessiter d’artefacts comme les commentaires et les régions. La clé réside dans le nommage et le découpage des méthodes et des variables. L’écriture d’un commentaire représente un échec à produire du clean code.

Il faut néanmoins relativiser. Cela n’est pas non plus toujours dramatique. On sait bien que les commentaires du type // TODO, // FIXME sont temporaires et à traiter ultérieurement, en les remplaçant par le code attendu ou en les supprimant s’ils sont obsolètes. De même, une expression régulière peut vite se révéler complexe à comprendre sans l’aide d’un commentaire.

Ce rappel théorique fait, revenons-en au kata et à son exécution par oncle Bob. On constate qu’il y a peu de commentaires dans le code de ce kata. Il y a quand même un passage notable : oncle Bob se sert du commentaire d’un bloc de code, // Determines the amount for each line, comme base pour nommer la méthode extraite de ce bloc : DetermineAmount(). Il dit lui-même que c’est dangereux de croire un commentaire. Mais c’est comme un mauvais nommage. Cela représente déjà un certain niveau de connaissance du système, celui des auteurs tel qu’ils ont pu le retransmettre en écrivant le code. En soi, c’est déjà une base, peut-être imparfaite mais on ne part pas de zéro. C’est à nous d’améliorer le code, le rendre plus propre, par exemple avec un nommage meilleur ou porteur intrinsèque de la connaissance d’un commentaire avéré, validé et que l’on peut supprimer.

Fonctions courtes

La petitesse des fonctions est importante en clean code. Pour donner un ordre d’idée, 10 lignes est un maximum, entre 2 et 5 lignes est idéal, le tout étant visible à l’écran sans avoir à faire défiler (scroll) à droite ou à activer le retour automatique à la ligne (word wrap). Mais le nombre de lignes n’est qu’une indication, ce n’est pas un critère déterminant.

On parle de fonction courte/petite (small function) quand la fonction fait une et une seule chose (One Thing). Un if, un for, un try / catch font chacun déjà une seule chose. Leur contenu doit se limiter à appeler une autre fonction. Cela permet à oncle Bob de se passer d’accolade {} (en Java) afin d’épurer le code. Lorsque l’on est habitué à mettre systématiquement les accolades pour contrer un problème d’indentation trompant la compréhension du code, cela nous taquine. En même temps, le code est plus concis sans accolade, encore plus en C# où, par convention et à la base par symétrie, chaque accolade est mise à la ligne. On peut aussi se laisser inspirer par d’autres langages comme le F# ou le Python qui n’ont pas d’accolade et se basent sur le niveau d’indentation. Cela oblige à être rigoureux sur l’indentation et je trouve cela préférable.

Une fonction longue est un smell. Il est certain qu’elle fait plusieurs choses et cache d’autres fonctions plus petites. On utilise alors le refacto Extract Method. On aboutit à une arborescence de petites fonctions. À un certain stade, une fonction trop longue fait même autant qu’une classe respectant SRP. On peut alors utiliser le refacto Extract (into) class.

Dans tous les cas, cela introduit un nouveau niveau d’abstraction que l’on va nommer. Ce nommage, lorsqu’il décrit bien le What (le concept / l’abstraction) et pas le How (le détail d’implémentation), permet d’améliorer l’expressivité du code, de mieux communiquer les intentions. Cela peut également révéler de la duplication de code ou de concepts, et nous permettre de plus se conformer au principe DRY. Une fonction est courte lorsque tout son contenu est du même niveau d’abstraction, ce qui facilite la compréhension du code.

Jusqu’où aller dans ce découpage ? Vu que le nombre de lignes n’est qu’indicatif, quand atteint-on ce “One Thing” ? Oncle Bob emploie l’expression “Extract till you drop” qui incite à creuser au maximum, à poursuivre le découpage le plus finement possible. Tant qu’on arrive à trouver un What différent, cela fait sens et a de la valeur. On aboutit alors à un code global plus long en nombre de lignes et c’est bien : les concepts implicites voire inconscients sont désormais explicites. Le débroussaillage et le balisage du chemin est fait pour le suivant. L’effort du lecteur est plus consciemment porté sur la connaissance et la compréhension des concepts et de leurs liens. Alors, la logique du programme est plus facile à comprendre.

Oncle Bob applique ce raisonnement en s’attaquant à la méthode Statement.generate(). Elle fait 45 lignes. Il n’y a donc aucun doute sur le fait qu’elle fasse trop de choses. La méthode est dure à comprendre, plusieurs niveaux d’abstraction étant mélangés. Après plusieurs extractions de méthode réalisées de haut en bas, oncle Bob arrive à la décomposition suivante :

☝️ Note
Au total, on a presque doublé le nombre de lignes de code en passant à 77 lignes. Cet aspect peut ennuyer les débutants. Pourtant, c’est inéluctable : les fonctions créées révèlent des abstractions qui étaient auparavant implicites, probablement seulement dans la tête du développeur. Cela fait plus de code à lire mais sa lecture nécessite moins de travail intellectuel de compréhension, les abstractions étant nommées et organisées par niveau descendant.

Organisation des fonctions

Dans la décomposition précédente, remarquez l’ordonnancement des fonctions (Function Structure) : il suit la Step-down Rule. Les fonctions sont triées en suivant l’ordre de leur appel, de manière récursive c’est-à-dire par niveau d’abstraction descendant. Plus on descend dans le fichier, plus on entre dans le détail d’implémentation des fonctions appelées plus haut. Cela facilite la lecture qui se fait comme celle d’un article de journal : les généralités d’abord puis le détail. La proximité créant du lien, je trouve cette organisation plus cohérente que le classement par ordre alphabétique auquel j’adhérais auparavant. Il faut juste faire attention à bien paramétrer ses outils de formatage du code pour que cela soit compatible avec la step-down rule, comme constaté par l’auteur de cette question sur Stack Overflow.

Un autre point en rapport avec l’organisation des fonctions : la répartition des fonctions entre différentes classes. On cherche de la cohérence au sein d’une classe, par rapport à ses données et entre ses fonctionnalités. Il est préférable de placer une fonction au plus près des données qu’elle utilise, permettant de les encapsuler dans la classe. Oncle Bob tombe successivement sur deux occasions d’améliorer le design en ce sens.

rental.getMovie().getTitle() est une violation de la Loi de Demeter que l’on résume en : « ne pas connaître les voisins des voisins ». Chaque classe ne communique qu’avec les classes qu’elle connaît. C’est un principe de connaissance minimale et cela permet de réduire le couplage. Pour corriger le code précédent, on cherche à avoir directement rental.getTitle(). Il suffit donc créer la méthode getTitle() { return movie.getTitle(); }. C’est du simple Forwarding, souvent appelé à tort Délégation.

Lorsqu’une méthode/fonction utilise plus les données d’une autre classe/module que de celle/celui la contenant, cela sent la Feature Envy. C’est le cas de determineAmount(rental) et determineFrequentRenterPoints(rental) qui utilisent les champs de l’objet rental plutôt que de leur classe Statement. Oncle Bob utilise alors le refacto Move Instance Method en spécifiant Rental comme type de destination, celui-ci étant proposé par l’IDE par analyse des types utilisés dans la méthode. Cela veut dire que le refacto marche aussi pour des champs de la classe.

Refacto du switch

Oncle Bob applique le refacto Replace Conditional with Polymorphism pour remplacer le switch dans determineAmount() et determineFrequentRenterPoints() par un polymorphic dispatch i.e. par l’appel à movie.determineAmount() où la méthode est abstraite et définie dans le type Movie mais implémentée dans des classes héritant de Movie. Cela permet de suivre le DIP , (le D de SOLID).

👉 Note
Le polymorphic dispatch est un dynamic dispatch : ce n’est qu’au runtime qu’est connu le type concret de l’objet et donc l’implémentation de la méthode virtuelle, localisée dans l’une des classes de la hiérarchie définie par héritage. Les langages orientés-objet nous offrent donc une certaine souplesse à même de satisfaire l’OCP(le O de SOLID), le tout avec sécurité : on est assuré que la méthode existe.

⚠️ Attention
Ce n’est pas la même chose dans ces deux cas-ci :
– En C#, les méthodes d’extension permettent d’ajouter des méthodes à un type, enum et interface inclus. C’est pratique de pouvoir écrire o.Method() plutôt que Method(o), surtout pour enchaîner les appels comme avec les requêtes LINQ : items.Where(...).Select(...).First(). Cependant, ce n’est que du sucre syntaxique. Les méthodes d’extension ne sont pas virtuelles et donc ne peuvent pas participer à un polymorphic dispatch. Pour le retrouver, on s’orientera plutôt vers le pattern décorateur.
– Les langages dynamiques, y compris C# avec le type dynamic, permettent d’appeler un membre d’un objet non connu à la compilation. Cela autorise encore plus de souplesse mais au détriment de la sécurité offerte par le compilateur. En C#, en alternative au pattern Visiteur et son « double dispatch », on peut utiliser une conversion en dynamic</codeem>(ligne 12)</em> pour éviter une série de <em>down casts</emem>(lignes 4, 5)</em> : </p></blockquote> <pre><code class="language-cs" highlight="4,5,12"> // Down casts même si avec pattern matching (syntaxe plus élégante qu’avant) void Print(Shape shape) { switch (shape) { case Square s: Print(s); break; case Circle c: Print(c); break; default: // Gestion explicite des autres types } } // Dynamic dispatch (unsafe pour les types non gérés) void PrintDynamic(Shape shape) { Print((dynamic) shape); } void Print(Square square) {...} void Print(Circle circle) {...}

Pour effectuer le refacto Replace Conditional with Polymorphism, oncle Bob crée autant de classes héritant de Movie que de cas dans le switch : NewReleaseMovie, ChildrensMovie, RegularMovie. Pour cela, de nouveau il bascule côté tests pour piloter le design du code de production. L’astuce consiste à changer l’initialisation des champs de type Movie : au lieu d’appeler le constructeur de la classe de base, Movie, on appelle le constructeur de la future classe. Cela permet de la créer en s’appuyant sur l’IDE pour automatiser cette tâche.

Exemple pour le champ childrens :
– Avant : childrens = new Movie("Childrens", Movie.CHILDRENS)
– Après : childrens = new ChildrensMovie("Childrens")

ChildrensMovie n’existe pas encore. L’IDE le colore en rouge et propose un quick fix pour créer la classe ChildrensMovie qui dérivera déjà de Movie du fait du type du champ childrens. Il suffit alors de compléter le constructeur pour appeler celui de base en spécifiant le title et le priceCode désormais implicite :



    public class ChildrensMovie extends Movie {
      public ChildrensMovie(String title) {
        super(title, Movie.CHILDRENS);
      }
    }

💡 Astuce
De cette manière, le temps passé avec les tests en rouge est minime. Très vite, on revient en vert et on récupère notre système opérationnel. Évidemment, cela suppose de jouer les tests très régulièrement, une bonne pratique à acquérir.

Oncle Bob effectue alors un Push Members Down des méthodes determineXxx() en sélectionnant l’option Keep abstract pour conserver la méthode abstraite dans Movie. Dès lors, chaque classe dérivée de Movie récupère une copie complète de ces méthodes, y compris les switch/if. Ne reste plus qu’à les épurer des cas qui ne les concernent pas. Pour cela, oncle Bob active la couverture des tests pour visualiser le code non couvert qui peut être supprimé après vérification (Souvenez-vous du cas non couvert par les tests). Enfin on peut supprimer le champ priceCode et les constantes correspondantes telles que CHILDRENS, les types dérivées de Movie portant intrinsèquement cette information.

Le refacto du Replace Conditional with Polymorphism nécessite donc plusieurs opérations à la suite. Il faut orchestrer différents refactos gérés par l’IDE, guidés par de subtiles modifications manuelles du code. Je trouve la façon de faire d’oncle Bob assez efficace. Il y a sûrement d’autres procédés possibles, peut-être plus efficaces, d’autant que les refactos gérés par nos IDEs sont de plus en plus perfectionnés.

☝️ Note
On retrouve le pattern Template Method dans sa version classique qui s’appuie sur l’héritage : les méthodes determine*() de Movie sont abstraites et implémentées dans les classes dérivées. On n’a qu’un seul niveau de profondeur et les classes dérivées ne font rien d’autre qu’implémenter les méthodes abstraites de leur classe mère. C’est donc une utilisation sécurisée de l’héritage. Néanmoins, qui dit héritage, dit problèmes en germination. Pour éviter cela, deux choix s’offrent à nous :
– La classe Movie ne contient que le champ title. Cela représente la mutualisation de vraiment peu de code. Il est donc envisageable de transformer Movie en pure interface et de laisser les classes l’implémentant initialiser le title. La duplication de code, très faible, est acceptable.
– On peut s’orienter vers un autre pattern comportemental s’appuyant lui sur la composition, la Strategy. On aurait alors une ou deux classes permettant de déterminer le montant et les points de fidélité selon différentes stratégies : films pour enfants, nouvelles sorties ou standard. Autre avantage : pouvoir mutualiser les instances de stratégie(s) entre les films.

Résultat

Design

Oncle Bob a adressé bon nombre des problèmes que l’on avait mentionnés. Le code et le design obtenus respectent les principes clean code, le tout en 1h !

  • Toutes les fonctions sont courtes, simples, expressives : leur nom exprime ce qu’elles font (le What), pas comment elles le font (le How).
  • Le comportement est mieux réparti entre les classes.

Diagramme de classe - Version refactorisée par Oncle Bob

Il reste des points améliorables que l’on a évoqués précédemment : couplage temporel, encapsulation ébréchée, usage de l’héritage. Nous les traiterons dans un prochain article.

L’art d’utiliser son IDE

Oncle Bob nous montre une succession de techniques de refactoring effectuées dans IntelliJ. C’est du grand art dont il faut s’inspirer, y compris en l’observant faire encore quelques opérations manuelles, automatisables dans l’IDE 😜.

☝️ Côté .NET
On a le choix : ReSharper ou CodeRush intégré à Visual Studio, le petit nouveau Rider, IDE qui offre une UX différente de Visual Studio, plus de stabilité (qui ne connaît pas les gels de VS qui désespérément ne répond plus !?), mais qui gagnerait à pouvoir lancer des extensions de Visual Studio. Visual Studio et VSCode ne sont pas en reste en proposant régulièrement des fonctionnalités déjà offertes par les concurrents.

Malgré quelques imperfections, les IDEs modernes nous offrent un gain de productivité et de sécurité, pour peu que l’on acquière la discipline de les utiliser et de manière efficace : en utilisant les raccourcis clavier. On n’est pas obligé de connaître tous les refactos possibles. Pour commencer, on peut se limiter à ces 6 refactos essentiels : Rename, Inline, Extract Method, Introduce Variable, Introduce Parameter, Introduce Field. L’important est de s’appuyer sur un ou plusieurs IDEs et de maîtriser les fonctionnalités essentielles pour le refacto sécurisé et une productivité boostée.

Cela n’empêche pas de chercher à découvrir d’autres fonctionnalités. À ce titre, je vous en partage deux que je pense peu connus et que j’utilise avec plaisir voire jubilation en pensant aux fastidieuses tâches répétitives que cela m’épargne :

  • Vous connaissez probablement l’édition en colonne. Son prolongement naturel est l’édition multi-curseurs. Elle se fait soit au moyen de clics à la souris, imprécis (mais on n’a parfois pas le choix, cf. GIF animé ci-dessous), en se basant sur la sélection courante et en ajoutant les occurrences à la sélection qui se segmente en plusieurs bouts de code éditables conjointement, indépendamment de leur positionnement en colonne. Cette fonctionnalité est disponible entre autres dans les produits de JetBrains (IntelliJ, Rider…), Visual Studio Code et Visual Studio 2017 depuis l’update 15.8, enfin ! Le seul point négatif : la terminologie et les raccourcis clavier sont différents selon chaque éditeur de texte.

Edition multi-curseurs

  • La complétion de code est une fonctionnalité classique. On la connaît avec « l’IntelliSense » et les snippets. Une facette moins connue est la « postfix completion ». Son action est rétroactive, en s’appliquant sur le bloc de code précédant le curseur pour le transformer : comme condition d’un if, en tant que variable extraite, booléen inversé… La postfix completion s’effectue en tapant un . suivi du code du template comme pour un snippet, respectivement .if, .var, .not pour les trois exemples précédents. Son objectif est d’éviter des retours en arrière du curseur. JetBrains propose la postfix completion depuis 2014 dans ses produits phares : IntelliJ, ReSharper, Rider. Je ne connais pas d’autre produit qui le fasse. C’est donc une manière de compléter du code. À noter des petits imprévus en l’utilisant, le bloc de code attendu n’étant finalement pas celui pris en compte.

Conclusion

L’étude du kata VideoStore et sa réalisation par oncle Bob a tenu ses promesses en se montrant riche d’enseignements non seulement sur les code smells et le refacto de code legacy, le bon usage des IDEs, mais aussi sur le clean code, les tests unitaires, le TDD, la programmation orientée-objet, les principes SOLID et les design patterns.

On a également évoqué la problématique de savoir quand arrêter le refacto. Un équilibre est à trouver entre le temps dont on dispose, les exigences et objectifs de qualité définis sur la base de code (codebase) par soi-même ou encore mieux partagés par l’équipe de développement. Oncle Bob s’est donné une heure et réalise une belle performance : si je devais faire la revue de code du pull request (PR) correspondant, je pourrais au plus émettre quelques suggestions et valider la PR.

Quand un développeur (soi-même, un autre développeur, voire une personne non technique) passera sur ce code, il devrait être plus à même de comprendre cette version du code plutôt que la précédente. On applique la règle du boy-scout de manière intelligente, en se projetant dans le futur, en comprenant l’appartenance collective du code.

En même temps, après avoir évoqué ces améliorations possibles, en restant dans le cadre d’un exercice porteur d’enseignements, je vous propose de continuer le refacto, en C# d’un côté mais également en TypeScript. Ce sera à mon tour de ne pas aller trop loin. La suite dans un prochain article.
En attendant, j’espère que vous avez apprécié ces articles. Si cela induit en vous de nouvelles pratiques, un état d’esprit de craftsmanship, mon objectif de diffuser ce que l’on a vu au sein des masterclasses clean code sera atteint 😄

© SOAT
Toute reproduction est interdite sans autorisation de l’auteur.