Débutant

Clean Code avec le Kata Video Store » Analyse préliminaire

Dans cette série d’articles, nous allons parler de Clean Code avec des cas concrets en C# mais aussi en TypeScript.

Ts

J’apprécie beaucoup ces deux langages et il m’arrive de trouver TypeScript encore plus élégant que C#. C’est particulièrement le cas avec l’inférence de type et la programmation fonctionnelle, certes pas aussi aboutis qu’en F#, Haskell… Or, TypeScript n’est pas toujours bien utilisé, ressemblant soit à du JavaScript saupoudré de types, soit à du C# ou Java. D’autre part, on parle peu de Clean Code en TypeScript. Au sortir de ces articles, j’espère que vous aurez envie de faire du Clean Code y compris en TypeScript.

Pour ancrer tout cela dans des cas concrets, nous allons partir de Video Store, le code kata issu du fameux livre Refactoring, Improving the Design of Existing Code de Martin Fowler, datant de 2002 et toujours d’actualité, contrairement aux magasins de location de vidéos 😆 Ce kata est joué lors de la masterclass[1] Clean Code[2], proposée par la communauté Craftsmanship de Soat dont je fais partie et inspirée par le livre Clean Code et les vidéos Clean Coders de Robert C. Martin alias Uncle Bob. Dans l’épisode 3. Functions, il effectue ce kata en Java.

Nous allons voir le code original du kata, porté en TypeScript et en C#. Nous y verrons quelques code smells. Dans les prochains articles, nous étudierons la version refactorée d’Uncle Bob. Nous pousserons ensuite le design un peu plus loin, à la fois en C# et en TypeScript, ce qui sera propice à en faire un petit comparatif. Je terminerai par un bref retour d’expérience sur Rider, le dernier IDE de JetBrains que j’ai utilisé à la fois pour C# et TypeScript.

Sommaire:Uncle Bob/Video Store/Tests unitaires/Code de production· 20 min

Uncle Bob

Bob

On ne peut pas parler de Clean Code sans présenter Uncle Bob. Pour ceux qui ne connaîtraient pas ce chaleureux personnage, il s’agit de l’un des signataires du manifeste Agile de 2001, avec Martin Fowler, Kent Beck… Il est à l’origine du manifeste Craftsmanship élaboré en 2008 pour compléter le manifeste Agile. Il est le promoteur des principes SOLID, du Clean Code, du professionnalisme, d’une Clean Architecture, au travers de ses conférences, de ses vidéos et de ses livres :

  • Agile Software Development, Principles, Patterns, and Practices (2002)
  • Clean Code: A Handbook of Agile Software Craftsmanship (2008)
  • The Clean Coder: A Code of Conduct for Professional Programmers (2011)
  • Clean Architecture: A Craftsman’s Guide to Software Structure and Design (2017)

Voici un extrait de son interview récente (novembre 2017) nous permettant de situer cette série d’articles dans la lignée d’Uncle Bob :

» Do you practice TDD and if so, what are some of your favorite code katas?
Of course I do. I often do the Video Store Kata that Martin Fowler made famous so many years ago.
I also do the Bowling Game, Prime Factors, Word Wrap, Stack, Environment Controller, and many others.

» What are some of your must-have tools or libraries that you use in your daily work?
A good unit test library and the JetBrains suite of IDEs and plugins.

Video Store

Présentation

Le “Video Store” est un kata de refactoring. On y travaille à améliorer un code existant grâce aux principes Clean Code : le nommage et l’organisation des variables, méthodes et classes, la taille des fonctions (small function, do one thing), la programmation orientée objet (POO)

Cet exercice permet en particulier de :

  • Se frotter aux switch considérés en POO comme « une occasion manquée d’utiliser du polymorphisme. »,
  • Appliquer les principes DRY, à contrebalancer avec DAMP dans les tests (pour favoriser leur expressivité tant que la duplication de code y reste légère), KISS, YAGNI (ne pas aller trop loin dans les refactos).

Démarrage

👉 Le code est disponible sur ce GitHub.

On commence l’exercice par l’analyse du code en TypeScript et plus particulièrement par ses tests unitaires. Cela permet de saisir plus rapidement la logique métier. On passera ensuite au code de production pour comprendre l’implémentation et identifier les code smells, les parties à refactorer pour obtenir du code plus propre.

Tests unitaires

Vérification de leur statut

Premier réflexe à avoir sur ce type de katas de refacto : jouer les tests unitaires (TU) pour savoir s’ils fonctionnent. Un autre réflexe sera de faire fréquemment des commits locaux, afin de pouvoir revenir en arrière facilement au besoin.

Commençons par un petit tour sur le package.json pour savoir de quelles commandes on dispose :

  • test : la commande classique, exécutable dans un terminal avec npm run test, npm test, npm t. Elle lance le framework de test Jest.
  • tdd est la commande équivalente en mode “watch”.
  • cover est associée à jest --coverage pour produire un rapport sur la couverture du code par les tests.

La commande npm t produit le résultat suivant, confirmant que tous les tests sont au vert :

PASS  src\videostore.spec.ts
  VideoStore
    ✔️ test single NewRelease statement (10ms)
    ✔️ test dual NewRelease statement (1ms)
    ✔️ test single children statement (1ms)
    ✔️ test multiple regular statement (1ms)

Test Suites: 1 passed, 1 total
Tests:       4 passed, 4 total
Snapshots:   0 total
Time:        6.766s
Ran all test suites.

Cela nous permet aussi de constater que l’on a 4 cas de tests, tous localisés dans le fichier videostore.spec.ts :

  • Single NewRelease statement
  • Dual NewRelease statement
  • Single children statement
  • Multiple regular statement

Couverture par les tests

Avant de refactorer du code de production, mieux vaut s’assurer que les tests couvrent tout ce code. Oncle Bob compare une bonne suite de tests à un parachute. En effet, il faut avoir confiance autant en un parachute lorsque l’on saute d’un avion qu’en sa suite de tests lorsque l’on modifie le code de production. L’indicateur le plus facile à obtenir pour jauger la qualité des tests est le taux de couverture : avec 100%, on est sûr de ne pas avoir de trou dans notre parachute.

Vérifions cela en lançant la commande npm run cover i.e. jest --coverage :

[...]
-------------|----------|----------|----------|----------|-------------------|
File         |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
-------------|----------|----------|----------|----------|-------------------|
All files    |    97.87 |    90.91 |      100 |    97.67 |                   |
 customer.ts |    97.06 |    90.91 |      100 |    96.88 |                35 |
 movie.ts    |      100 |      100 |      100 |      100 |                   |
 rental.ts   |      100 |      100 |      100 |      100 |                   |
-------------|----------|----------|----------|----------|-------------------|
Test Suites: 1 passed, 1 total
Tests:       4 passed, 4 total
Snapshots:   0 total
Time:        6.355s
Ran all test suites.

Il y a trou dans les tests. La ligne 35 de customer.ts n’est pas couverte (cf. ci-dessous). La première à chose à faire lors du kata sera donc de rajouter un TU pour couvrir ce cas.

    case Movie.CHILDREN:
        thisAmount += 1.5;
        if (each.daysRented > 3) {
            thisAmount = thisAmount + (each.daysRented - 3) * 1.5;
        }

Golden master

En pratique, il peut être très/trop coûteux d’atteindre une couverture de 100% avec des tests unitaires de qualité. Sur certains codes legacy, rien que mettre en place le premier test peut être un gros challenge, du fait de couplage à une base de données, à un environnement, à un framework… tout ce qui est difficile à mettre en place manuellement dans un test unitaire ! Pour mettre en place ces TU, on risque d’introduire des bugs dans le code de production : un comble !

L’un des moyens pour s’en sortir est alors de réaliser des tests de haut niveau, jetables, en capturant un Golden Master au format texte, par exemple avec le framework ApprovalTests : pour plus d’informations, regardez cette vidéo où Johan Martinsson, récemment passé à Soat pour animer le TechLab Bugs Zero Kata du 21 mars 2018, utilise l’outil sur du code JavaScript, l’un des nombreux langages supportés.

Analyse d’ensemble

En matière de logique métier exprimée par ces tests, on constate qu’ils concernent tous le même SUT : la classe Customer et sa méthode statement(). Leur finalité est de produire un rapport concernant les locations de films d’un client, le montant dû et les points de fidélité acquis. Chaque film a un code prix parmi CHILDREN, NEW_RELEASE et REGULAR qui influe sur le montant et les points associés.

En matière de qualité du code, il y a déjà beaucoup à dire sur ces tests, au niveau de leur lisibilité, des duplications, des responsabilités. Voyons cela point par point.

Organisation des fichiers

Il est conseillé de marquer clairement une séparation entre le code de production et les tests. Cela passe a minima par des fichiers différents.

En JavaScript/TypeScript, l’une des conventions usuelles est de placer le fichier de tests dans le même répertoire que le fichier du SUT. La distinction s’opère alors au niveau de l’extension, ici .ts / .spec.ts.

En C#, il est plus standard d’avoir un projet de test par projet de production. On peut en plus regrouper les projets de production dans un dossier src et ceux de tests dans tests. Pour un kata ou un micro projet, c’est souvent trop et on peut s’en passer. C’est le parti que j’ai pris dans le code source.

Nommage des fichiers

Le choix de l’extension .spec.ts pour un fichier de tests unitaires est conventionnel, et approprié quand on utilise un système d’assertions de type “BDD-like”. C’est le cas ici, avec l’usage des fonctions describe() et it() de Jest.

En revanche son nom est trop générique. On préfère le format {SUT}.spec.ts qui donnerait ici customer.spec.ts. À noter que l’on pourrait avoir un nom de fichier de tests commençant par videostore s’il s’agissait de tests d’acceptation (TA) de la user story “VideoStore”, par exemple un fichier videostore.feature avec des TA écrits en Gerkhin et exécutables avec Cucumber.js.

Identification des cas de tests

La même problématique se pose avec le describe('VideoStore', ...) global : il fournit une description imprécise. Dans le cas de TU, indiquer le SUT est plus conventionnel pour permettre de mieux identifier les cas de tests parmi ceux qui échouent.

Dans cette optique, on peut également ajouter un sous-niveau pour indiquer la méthode testée. C’est utile lorsqu’il commence à y avoir beaucoup de tests pour une même méthode. Les résultats des tests sont alors mieux organisés en groupes et donc plus lisibles. Ici, cela se traduirait par ajouter un sous-niveau describe('statement()', ...). En C#, cela pourrait se faire avec une classe StatementShould imbriquée dans la classe CustomerTests.

On peut aussi opter pour un “phrasé” plus naturel des cas de tests en sortie, comme “customer should produce a statement with this property given these inputs” et “customer should support this edge case“. Alors, nous n’avons pas ce niveau intermédiaire de regroupement des tests.

Nommage du SUT

Le nom de la variable pour le SUT, customer, est traditionnelle vu son type Customer. De mon côté, je préfère désormais le nom sut permettant d’identifier immédiatement le SUT quel que soit le fichier de tests lu. D’autres, comme Sandro Mancuso, craftsman connu et respecté, préféreraient le premier type de nom, customer, plus métier que technique. Les deux choix se valent selon vos priorités.

Choix des valeurs

Autant le describe('VideoStore', ...) est un peu imprécis, autant donner la valeur “Fred” au client dans customer = new Customer('Fred'); est une précision superflue et donc parasite. Ce paramètre n’influe pas sur l’algorithme. On le retrouve juste dans la ligne d’en-tête en sortie : Rental Record for Fred.

Nous avons le même problème avec les titres de films qui font référence à des films existants : The Cell, The Tigger Movie… Cela vient parasiter la compréhension du test : on essaie de comprendre quel impact cela a. Il n’y en a pas, hormis dans le détail en sortie mentionnant les films loués. Les informations importantes pour les différents cas de tests sont le type des films et leur code prix.

Dans ces deux cas, il est plus judicieux d’éviter des valeurs réelles pour préférer des valeurs plus génériques, anonymes, par exemple :

Paramètre À éviter À préférer
Customer name 'Fred' ✔️ 'Any Customer Name'
Movie title 'The Cell' ✔️ 'Regular 1'

💡 En C#, on peut utiliser des librairies comme AutoFixture pour créer (entre autres possibilités) des valeurs aléatoires et de facto anonymes. L’inconvénient est que cela peut rendre plus difficile la compréhension d’un test qui échoue : il faut s’habituer à lire des "f5cdf6b1-a473-410f-95f3-f427f7abb0c7" dans la sortie des tests.

Nommage des cas de tests

Le nommage des cas de tests est de la forme it('test {RentalCount} {PriceCode} statement', ...). Cela doit se lire comme de l’anglais : “It …”. Donc avec un verbe conjugué à la troisième personne du singulier. Le verbe “test” est à la fois redondant et inapproprié, le sujet (“it”) étant le SUT ou sa méthode dans le cas du “sous-describe”.

On rencontre fréquemment la forme “it should {verb} …”. Cela permet de mettre le verbe à l’infinitif. On peut également se passer du “should”, mais il faut alors conjuguer le verbe à la troisième personne du singulier au présent, i.e. sans oublier le “s” pour les verbes réguliers.

Je trouve intéressant de nommer un test en ayant en tête les 3 étapes “Given, When, Then” de BDD, ce cas de test pouvant être aussi bien technique que métier. Le When et une partie du Given sont déjà fournis par les describe. Dans les 4 tests initiaux, on a bien le Given mais pas le Then : on ne sait pas dire ce qui va changer en sortie entre ces différents cas de tests. Il est préférable de l’indiquer dans le nom des tests, pour éviter au lecteur de devoir analyser le code à l’intérieur des tests pour le découvrir. Cela fournit par ailleurs un peu de documentation sur les règles métier.

Lisibilité du code dans les tests

On n’est pas gêné par une phase “Arrange” trop longue, travers courant dans les TU. Elle consiste en 1 à 3 appels à customer.addRental(new Rental(new Movie(...));. Par contre, il y a de la duplication de code. Appeler une seule fonction combinant ces 3 actions serait plus simple à utiliser et à lire.

Ce qui nuit le plus à la lisibilité est la valeur des “expected”, une longue chaîne qui oblige à scroller horizontalement : expect(customer.statement()).toBe('Rental Record for Fred\n\tThe Cell\t9.0\nYou owed 9.0\nYou earned 2 frequent renter points \n');. On peut améliorer la lisibilité en marquant des retours chariot au niveau des \n.

Le plus gros smell concerne justement le fait que l’on teste ces longues chaînes. Or, la qualité du code des tests est aussi importante que celle du code de production. Par définition, le refactoring sert à améliorer le code sans modifier le comportement du logiciel. Le refactoring est au cœur du clean code. Les tests sont donc notre garantie contre la régression pouvant survenir lors de refacto comme de l’ajout de fonctionnalités.

Le clean code s’applique donc aussi aux tests. Il est important d’avoir des tests :

  • Lisibles : les tests indiquent ce que fait le code de production, comment on s’en sert. Oncle Bob parle de living low-level documentation.
  • Maintenables : les tests doivent pouvoir être modifiés facilement pour suivre les changements du besoin métier.
  • Simples : la complexité doit se trouver côté code de production. Les tests ne doivent pas en rajouter. Il est ainsi conseillé de garder la complexité cyclomatique à 1, c’est-à-dire de ne pas avoir de if.

👉 Lors d’un kata de refacto, ne pas hésiter à refactorer les tests aussi ! Il ne faut pas rester avec un test avec lequel on n’est pas à l’aise alors que l’on aura de cesse de l’exécuter et de lire son résultat. Le but est aussi de s’entraîner au TDD, ce qui signifie commencer par les tests y compris concernant le refacto si le besoin s’en fait ressentir.

💡 C’est en jouant les tests unitaires qu’on les valide, que l’on se protège contre de la régression dans le code de production comme dans le code des tests. Pour cela, il faut les jouer très régulièrement, après chaque refacto, pour les voir passer en rouge si le code de production ne fait pas ce qui est attendu, en vert dans le cas inverse.

Revenons à nos TU actuels : ils testent à la fois le bon formatage en sortie et les valeurs calculées. La partie “formatage” correspond à du test d’interface. Ce type de test est plus haut dans la pyramide des tests :

Pyramide des tests

Il faut donc en faire (beaucoup) moins que les TU. Ces derniers s’intéressent à des valeurs plus simples à tester : ici les montants et les points. On pourra donc conserver 1 ou 2 de ces tests d’interface et convertir les autres en TU testant les montants et les points.

Code de production

Le code de production se répartit en trois classes, Movie, Rental, Customer.

Diagramme de classe - Version initiale

Les diagrammes de classes ont été générés depuis le code C# avec NClass, d’où le nommage en PascalCase et les propriétés X { get; set; }, équivalents des accesseurs getX() et setX().

Les classes Movie, Rental n’ont pas de méthode i.e. pas de comportement : ce sont des data classes. On parle aussi d’un modèle anémique en DDD. Dans tous les cas, c’est suspect.

Divers smells

L’essentiel de la logique métier est condensé dans la méthode Customer.statement() :

  • Fait plusieurs de lignes, correspondant au smell Long Method. Il serait préférable d’extraire des sous-méthodes : on pourrait alors, en lisant le code de statement(), identifier les différentes parties du rapport : en-tête, détail, totaux, sans se soucier du détail de leur implémentation. On équilibre ainsi les niveaux d’abstraction, suivant le Balanced Abstraction Principle de Sandro Mancuso.
  • Calcule tout à la fois :
    • les montants et points de chaque film, à l’aide d’un switch correspondant à l’un des smells Object-Orientation Abusers, détaillé plus bas,
    • leurs totaux,
    • le rapport global.
  • On gagnerait aussi à renommer la variable each dans la boucle for (let each of this._rentals), en rental par exemple. Sinon le nommage est correct. Sauf celui de la classe : Customer, qui ne reflète pas la responsabilité principale de la classe : la production du rapport.
  • On voit beaucoup de each.xxx, ce qui sent le feature envy : des fonctionnalités gagneraient à être rapprochées des données manipulées, le tout rassemblé dans une classe.
  • On a également droit à une ligne de commentaire, smell Comment dont on peut se passer avec un meilleur nommage.

Bloc switch

On y trouve enfin, comme attendu, un switch. Certes mot clé de nombreux langages, comme peut l’être goto, il n’est pas anodin pour autant, avec des conséquences à connaître pour les assumer ou en changer.

Chaque case du switch correspond à une branche, c’est-à-dire à un point de complexité cyclomatique. Un outil comme SonarQube indique qu’une valeur supérieure à 10 est problématique, ce qui est déjà beaucoup ! Mais cela monte très vite avec les switch, typiquement quand les case correspondent à des valeurs d’une enum. De plus, dès que l’on ajoute un cas tel qu’un membre d’une enum, il faut vérifier dans tous les switch comment traiter (ou non) ce nouveau cas. Cela complique la maintenance et disperse les règles de gestion dans différentes parties du code.

Tout comme les séries de if / else if, la fonction d’un switch est de faire un dispatch. Or celui-ci est fait à la main. Cela crée un couplage entre des portions de code de niveaux d’abstraction différents, le switch étant de plus haut niveau que ses sous-case. Oncle Bob parle de problème de fan-out en référence au nombre maximal de sorties d’un composant électronique :

Fan-out problem

Cela peut créer un nœud de dépendances : le switch va dépendre directement d’autres modules. Cela empêche le déploiement indépendant de chaque module : toute modification dans une de ces parties ne pourra être déployée qu’en déployant l’ensemble !

La solution en POO consiste à s’appuyer sur le polymorphisme pour réaliser un polymorphic dispatch et sur le principe d’inversion de dépendances. Le module contenant ce switch définit son besoin grâce à une interface que les autres modules implémentent. Dès lors, le switch peut être remplacé par un simple appel à une méthode d’un objet ayant la signature spécifiée :

Fan-out solution

En utilisant une classe abstraite plutôt qu’une interface, on retombe sur le design pattern Template Method. Oncle Bob arrive à ce design à la fin de son refacto, comme nous le verrons de manière plus approfondie dans la prochaine partie.

La fourniture de cet objet se fera dans le code appelant, soit directement, soit grâce à une Abstract Factory, cette dernière pouvant utiliser un switch pour créer l’instance du bon type mais cela sera alors l’unique switch de ce type dans l’application. C’est l’un des seuls, voire l’unique usage toléré de switch, vu que l’on ne peut pas faire autrement, sauf si on le fait faire de manière indirecte par un éventuel framework.

Conclusion

En peu de code, le kata VideoStore nous confronte à de nombreux smells. C’est toute la richesse de ce kata. Nous avons évoqué des principes de clean code pour améliorer un code existant ou structurer directement bien un code en cours d’écriture. Pour les débutants craft, cela peut faire beaucoup. Ne vous découragez pas, pas la peine de tout savoir, de tout faire parfaitement ! L’entraînement via les code katas sert à cela, avant de mettre en pratique chez les clients une fois plus aguerri-e-s.

Reste que coder directement en clean code est beaucoup plus compliqué. Un dernier petit conseil pour la route : mieux vaut ne faire qu’une chose à la fois : d’abord un code qui marche, ensuite un code propre, bien conçu et pour finir, au besoin, un code performant. C’est la philosophie derrière l’expression Make It Work, Make It Right, Make It Fast. Mieux vaut se garder d’optimisation prématurée tant au niveau du design que des performances. On parle aussi de baby steps, de KISS évoqué plus haut. Si l’on veut faire quelque chose d’intelligent, ça me semble une meilleure façon de faire plutôt que de chercher à faire compliqué. Tout ceci n’est pas nouveau : Voltaire disait déjà le mieux est l’ennemi du bien. Maniaque et perfectionniste comme je suis, cela m’a pris du temps (quasiment 2 ans !) à le comprendre et à l’appliquer régulièrement, y compris dans les moments de rush, sauf quand je m’égare 😜.

C’est aussi l’objectif d’un kata de refacto : s’entraîner à améliorer un code qui marche, pour que cela devienne une habitude. Devant une échéance à tenir, mieux vaut assurer la phase Make it work quitte à remettre à plus tard les deux autres phases, en ayant bien en tête la dette technique que cela représente, et en assurant une discipline suffisante pour la résorber en temps voulu. Les revues de code, par exemple lors de pull requests, sont aussi une bonne occasion de réaliser le Make It Right. Le remplacement du switch par du polymorphisme est typique de cette phase.

Pour asseoir ces conseils sur du concret, nous verrons dans les prochains articles la version refactorée d’Uncle Bob que nous “challengerons” en poussant le design encore plus loin.


Notes :

  1. Une masterclass est une formation au format 50% théorique / 50% pratique. Plus d’informations ici. ⤴️
  2. J’ai choisi de ne pas traduire Clean Code en français.
    • Si cette expression n’est pas encore passée dans le langage courant, j’espère qu’elle le sera bientôt. Je la trouve en effet plus élégante que les équivalents en français, l’expression désignant à la fois l’action, coder proprement, les principes sous-jacents et le résultat, un code propre.
    • Je devrais plutôt écrire un « code plus propre » car il n’y a pas d’absolu en la matière. C’est sujet à avis personnels. Mais il s’agit surtout de faire preuve de professionnalisme par pragmatisme : la pratique du Clean Code ne doit pas se faire aux dépends de livrer de la valeur, suivant le premier principe du manifeste agile sur lequel s’appuie le Software Craftsmanship . ⤴️

© SOAT
Toute reproduction interdite sans autorisation de l’auteur

Pour aller plus loin

Nombre de vue : 757

AJOUTER UN COMMENTAIRE