Tutoriel PHP : Revue de code : felixdorn/flash

Je vous propose aujourd'hui de découvrir un nouveau format de vidéo, la revue de code. Le principe de ce format est d'apprendre de nouvelles choses et d'améliorer sa manière de développer en analysant le code d'autres développeurs.

L'objectif de cet exercice n'est pas simplement de critiquer le code d'un autre, mais plutôt d'améliorer sa propre manière de coder en analysant les problèmes que l'on peut détecter sur un code que l'on ne connaît pas.

Le sujet

Pour cette vidéo nous allons analyser la librairie PHP felixdorn/flash. C'est une librairie qui permet de gérer des messages flash à afficher lors de certaines actions utilisateur (on est ici avantagé car on connaît le principe de fonctionnement de ce genre de classe).

Par où commencer ?

Lorsque l'on décide d'évaluer le code de quelqu'un d'autre il faut partir du plus général vers le plus précis. On analyse d'abord la structure du projet pour identifier certains problèmes. Dans le cas de la librairie présentée aujourd'hui la structure est correcte et on identifie facilement les points d'entrées.

• Le dossier src correspond à la racine du namespace de la librairie
• Le dossier test contient les tests unitaires et permet de s'assurer que la librairie fonctionne comme attendu.
• La présence d’un fichier .travis.yml indique l’utilisation de l’intégration continue.
• Un dossier examples contient des exemples pour découvrir le fonctionnement de la librairie. Ce dossier n'est pas forcément nécessaire étant donné que l'on a les tests qui peuvent suffire à comprendre les scénarios d’utilisation de la librairie.

Le fichier composer.json

On peut maintenant rentrer plus en profondeur et commencer à regarder le contenu de certains fichiers et on va toujours commencer par le fichier composer.json.
A l'intérieur de ce fichier on regarde si les namespaces sont correctement définis (dans le cas de la librairie que l'on analyse le namespace déclaré est trop générique \Felix au lieu de \Felix\Flash).
Ensuite, on regarde aussi si la version de PHP nécessaire au fonctionnement de la librairie est définie dans la partie require. Cet argument est souvent oublié et donne lieu à de nombreux problèmes lorsque les gens essaient d’installer la librairie sur des versions de PHP inférieure à celle attendue. Indiquer un numéro de version minimum permettra aux utilisateurs de recevoir une alerte lors d’une tentative d’installation via composer.

Le Code

L'étape suivante consiste à regarder le code sans forcément essayer d'en comprendre la logique et d'essayer d'identifier des pratiques incorrectes.

Dans le cas de la librairie que l'on évalue on notera simplement un manque de cohérence dans le nommage des différentes méthodes et aussi un manque de commentaires (ce qui va rendre difficile l'étape suivante où l’on va chercher à comprendre le fonctionnement des différentes classes et leurs rôles dans la librairie). En revanche le formatage du code semble correct et cohérent entre les différentes classes (ce qui permet une meilleure lisibilité du code mais aussi de rechercher plus simplement les choses en cas de problème).

On notera aussi la présence d’interfaces. Cela est en général un bon signe parce que cela indique que certaines parties de la librairie vont pouvoir être remplacée par des implémentations différentes.

Le fonctionnement

Après avoir observé la forme du code il va falloir essayer de le comprendre pour détecter des problème de logiques ou de structure.

L'absence de commentaires explicatifs va nuire à l’efficacité de cette étape car il est difficile d'identifier le rôle des différentes classes et de comprendre les choix de l’auteur. En y regardant de plus près on observe aussi un problème de structure avec deux classes qui possèdent des méthodes quasi identiques et il est difficile de comprendre le rôle que joue les 2 classes Flash et Flasher.

Aussi, la classe Flasher semble (sous certaines conditions) interagir directement avec la session. Ce choix va à l'encontre du single responsibility principle et il aurait été intéressant de séparer la gestion de la session dans une classe dédiée avec une interface associée afin de pouvoir séparer la logique et permettre à l'utilisateur final de remplacer plus aisément cette partie-là si nécessaire. Cela permettrait aussi de ne pas avoir à vérifier à chaque méthode de la classe Flash que la session est bien démarrée.

Breaking change & PR ?

On remarque ici une erreur de conception que l'on ne peux malheureusement pas corriger sans introduire des changements majeurs au niveau de la librairie. Dans cette situation on ne va pas nécessairement proposer des changements trop important à l'auteur, car cela casserait la rétro-compatibilité et obligerait la création d'une nouvelle version. Dans notre situation ici on va se contenter de proposer des changements mineurs et des améliorations qui ne cassent pas le fonctionnement actuel de la librairie.
On peut proposer à l'auteur de travailler sur une restructuration de la librairie mais il est important de créer une issue avant de commencer afin de ne pas travailler pour rien. Si l'auteur ne souhaite pas faire de changements majeurs il peut-être plus judicieux de créer une nouvelle librairie sur laquelle vous aurez la main.

Objectif

Cet exercice est intéressant parce qu'il permet de remarquer certaines erreurs que l'on ait peut-être amené à faire aussi. Lorsque l'on travaille sur une librairie ou notre propre code il est difficile parfois d'avoir du recul et d'identifier certaines problématiques. C’est en regardant le code des autres et en essayant de le comprendre que l'on peut trouver des techniques pour améliorer le nôtre.

Si vous voulez proposez votre code pour une prochaine revue de code, n’hésitez pas à venir sur le tchat (salon #code-review)