Les revues de codes dans notre équipe
Qu’est-ce qu’une revue de code ?
Le développement, c’est de l’écriture : on utilise un langage, on respecte des règles, une syntaxe, un style. Cela semble normal de procéder à une relecture dans le cadre de l’écriture d’un article de magazine ou avant de publier un livre. Alors pourquoi la revue de code dans les projets informatique n’est-elle pas systématique ? Elle le devrait.
Il s’agit non seulement de vérifier que le code semble correspondre au besoin mais également de veiller à sa qualité : lisibilité, clarté, consistance.
Quel est l’intérêt ?
L’intérêt des revues de code est triple :
- Détecter les défauts au plus tôt
- Diffuser de l’information aux membres de l’équipe
- Progresser en lisant du code qui n’est pas le sien
Détecter
La revue de code est complémentaire aux tests. Les défauts détectés lors des revues de code ne sont pas les mêmes que ceux détectés les lors des tests fonctionnels. Il s’agit de chercher d’éventuelles défaillances au plus tôt dans le cycle du développement. Plus un problème est détecté en amont, moindre sera son impact ; plus simple il sera à corriger, moins il coûtera cher à l’entreprise. Les tests unitaires font baisser de 25% les anomalies tandis que ce chiffre est plutôt de 55 à 60% pour les revues de codes (source étude Capers Jones)
Diffuser
La connaissance de l’existant sera diffusée beaucoup plus rapidement. Chaque développeur aura la connaissance du fonctionnement de chaque module et de ses différentes évolutions. Les estimations futures seront également plus précises. L’idée est de renforcer la connaissance globale du code ainsi que la propriété commune : le code n’appartient à personne. Pour les développeurs nouvellement arrivés dans l’équipe, cela permet également d’accélérer la courbe d’apprentissage.
Progresser
Échanger avec ses pairs permet toujours de monter en compétence. Un collègue aura une approche ou une vision différente, la connaissance d’un pattern peut-être mieux adapté ou saura simplement poser les bonnes questions pour améliorer le produit. Débattre des choix d’implémentation même si l’on ne modifie rien sur la Pull Request est souvent utile. Cela permet de prendre du recul sur ses choix et ses convictions. Personne n’a la science infuse, nous pouvons et nous devons apprendre en continue, le faire avec les autres permet d’optimiser l’apprentissage par l’échange : la revue de code est avant tout un moment propice aux interactions.
Quels sont les points d’attention ?
Au-delà des classiques remarques de nommage, de layout, de concepts (SOLID, KISS, YAGNI, DRY, Demeter, etc) et de réponse au besoin évoquée dans la story, il faut s’attarder sur certains points lors d’une revue de code :
- A-t-on de bons tests unitaires ?
- L’évolution créé-t-elle des breaking changes ? Pourquoi et peut-on l’éviter ?
- Le code est-il compatible avec les guidelines et conventions de l’équipe et de l’entreprise ?
- Le code est-il lisible, compréhensible, consistant ?
- Est-ce simple de comprendre l’intention de l’auteur ?
- A-t-on de l’optimisation prématurée ce qui rendrait le code inutilement complexe ?
- Les abstractions sont-elles au bon niveau ?
- Les patterns utilisés et l’organisation du code sont-ils judicieux ?
- Le code créé-t-il une faille de sécurité ?
- Pourrait-on avoir un problème de performance ?
- Pourrait-on rencontrer des problèmes difficilement reproductibles (fuites mémoires, gestion des erreurs, multithreading, deadlocks) ?
- La documentation doit-elle être mise à jour ?
Comment organisons-nous nos revues de code ?
Dans notre équipe nous utilisons Azure DevOps avec Git.
Une fois la Pull Request créée, le développeur envoie un message avec l’url sur le canal Slack de l’équipe. Une validation suffit à valider un merge sur la branche master mais dans l’idéal, nous préférons avoir à minima 2 reviewers.
Il s’agit d’une pratique technique : seuls les développeurs y participent (et donc ni les managers, chef de projets, product owner, etc.).
Nous connaissons et partageons sur un wiki nos conventions et guidelines techniques liés à l’équipe et à l’entreprise.
Nous essayons autant que possible de faire en sorte que la revue de code soit rapide et facile à lire. Il s’agit donc de bien découper les stories pour que la feature branch et donc la Pull Request soit légère avec une quantité de modification de code limité. La validation se réalise en général dans les quelques heures qui suivent la demande sur Slack.
Au niveau du format, nous utilisons pour la grande majorité des revues le processus asynchrone avec des commentaires sur l’outil. Il nous arrive cependant pour des Pull Requests plus complexes de la réaliser à 2 ou 3 sur le même écran, le développeur de la fonctionnalité expliquant alors sa proposition et répondant aux questions.
La revue de code arrive-t-elle trop tard ?
La revue de code est classiquement réalisée après le développement.
Le processus classique dans notre cycle de développement est le suivant : Story => Développement Git sur une feature-branch => Revue de code => Merge => Tests => Déploiement
Pour des stories simples où le besoin et la solution sont clairs pour tous les membres de l’équipe, cela ne pose pas de problème.
Mais ce processus n’est pas optimal dans plusieurs situations :
- Arrivée d’un nouveau développeur dans l’équipe qui doit monter en compétence sur le domaine business et l’existant
- Story complexe où la solution n’est pas claire
- Divergence d’opinion sur une technologie/un choix entre différents membres de l’équipe
Réaliser une revue de code à posteriori peut donc être contre-productif. Dans ces cas, il est nécessaire de réaliser une ou plusieurs revues avant ET pendant le développement.
- Réfléchir à plusieurs à priori nous permet de gagner un temps considérable lors de la revue de code finale réalisée à la fin du développement
- Plusieurs personnes auront déjà une vision claire du développement en cours, la revue sera plus qualitative
- Les débats structurants sur le code auront déjà eu lieu ce qui évite d’éventuels points de tension
- On limite la frustration engendrée par une éventuelle suppression de code
Pour certains sujets, on peut également aller plus loin et réaliser du Pair Programming ou du Mob Programming.
La revue de code c’est de la technique et… de l’humain
Nous ne sommes pas des machines. Nous pouvons faire des erreurs, nous en avons fait et nous en ferons encore. La pratique de la revue de code n’est pas de pointer du doigt un individu, ni de mettre en exergue les erreurs. Pour que cela fonctionne il est absolument nécessaire que l’état d’esprit soit positif :
- Respecter les principes de l’Egoless programming dont les principes les plus importants sont pour moi les suivants :
“Critiquer le code, pas les individus”
“Accepter de faire des erreurs”
“Tu n’es pas ton code (ne pas prendre les choses personnellement)”
- Accepter les compromis pour trouver une solution rapidement : parfois plusieurs solutions sont bonnes
- Être humble
- Éviter ce qui pourrait être mal interprété : sarcasmes, smiley dans certains contextes
Les différentes parties prenantes ont chacune leur responsabilité dans la réussite d’une revue :
- Les relecteurs doivent faire preuve de bienveillance
- Le développeur de la fonctionnalité doit savoir accepter ses erreurs et les retours de ses collègues.
En tant que relecteur
- Préférer poser des questions aux injonctions et demandes, demander des clarifications et des explications sur le choix d’implémentation
- Éviter les jugements
- Ne pas corriger code à la place d’un collègue derrière son dos : préférer proposer son aide pour une petite séance de Pair Programming
- Commenter le code, pas la personne
- Se mettre à la place de l’auteur et comprendre son point de vue, son contexte
- Si les échanges se multiplient sur un point en particulier, préférer une discussion d’équipe en face à face
- Éviter les rejets de Pull Request sur un outil : il est préférable de faire se rendre compte à l’auteur des améliorations à apporter. L’idéal dans ce cas est de réaliser une séance de Peer Review.
En tant que développeur de la fonctionnalité
- Ne pas négocier systématiquement
- Accepter avec enthousiasme de meilleures solutions
- Remercier les bonnes idées
- Répondre à tous les commentaires avant le merge
Exemples
Une code review qui ne se passe pas bien :
Jean-Michel : “c’est moche cette classe, elle fait trop de choses, en plus il y a du code dupliqué !!”
Roberto : “j’ai pas le choix, on a que 3 stories points sur cette US. Tu veux toujours faire de la surqualité, c’est du vieux legacy en plus ce projet”
Jean-Michel : “c’est pas parce que c’est du legacy qu’on peut pondre ce genre de code. Découpe au moins la classe en plusieurs”
……..
La même code review en respectant les bonnes pratiques :
Jean-Michel : “Je propose de découper cette classe pour la rendre plus lisible.”
Roberto : “C’est vrai qu’elle fait plusieurs choses, mais c’est compliqué de voir les responsabilités. Tu proposerais quoi ?”
Jean-Michel : “Pourrait-on factoriser les 2 fonctions du début avec des génériques ? Cela pourrait être ensuite réutilisé dans 2 classes une pour le calcul, l’autre pour le mapping ?”
Roberto : “Merci pour ton retour, je vais essayer ça et voir ce que ça donne.”
Le feedback
Il faut savoir accepter la critique et il faut également savoir la donner.
Un bon feedback respecte certaines règles afin qu’il soit efficace. Un commentaire mal formulé peut rapidement s’avérer contre-productif.
Un bon feedback est précis, spécifique et basé sur des faits observés.
Les 4 types de feedback
- Feedback de renforcement : il est positif et spécifique. Il permet de motiver et augmente l’estime de soi :
“Beau dev sur l’algorithme de calcul ! Tu l’as vraiment bien optimisé.”
- Feedback correctif/constructif : il est négatif et spécifique. Il s’agit d’une critique constructive pour solliciter une amélioration :
“J’ai constaté que tes 3 dernières PRs contenaient un breaking change sur ce webservice d’orchestration. J’aimerais que porte une attention particulière à cela”
- Feedback flatteur : il est positif et non spécifique. Il diminue l’estime de soi et le climat de confiance :
“Toi t’es le meilleur développeur la boite !”
- Feedback provocateur : il est négatif et non spécifique. Il diminue fortement l’estime de soi et aura un effet délétère :
“De toute façon, tu as toujours des dizaines de retour négatifs sur tes PRs”
Les feedbacks de renforcement et de correction/construction sont à utiliser régulièrement.
Les feedback flatteurs et provocateurs sont à proscrire.
Quelques règles essentielles
- Toujours garder un climat de confiance
- Rester neutre, on ne juge pas
- Ne pas surcharger d’information, aller à l’essentiel
- Se rattacher aux faits et non à la personne et aux comportements
Sources
Web
Livres
Donner et recevoir du feedback: Transmettre et recevoir des critiques constructives
