Comment rater vos revues de code ? – Épisode 1

Dans le précédent article, nous avons présenté la pratique de la revue de code ainsi que deux formats que nous utilisons sur nos projets.

Mais introduire une nouvelle pratique avec succès n’est pas une chose aisée. C’est un peu comme mettre une barque à la mer : une fois dans l’eau, les premiers mètres sont assez chaotiques. Il y a beaucoup de vagues, on commence à se demander si c’était une bonne idée. Ne serait-il pas plus sage de retourner au rivage ? Mais en persévérant, on arrive finalement au large, où la mer est plus calme : il suffisait de s’accrocher.

Nous avons rencontré dans nos expériences de revue de code plusieurs écueils qui nuisent aux bénéfices attendus et qui peuvent mettre en péril la pratique dans l’équipe.

Voyons au travers de quelques anecdotes vécues* pourquoi vos premières revues de code risquent d’être difficiles et quels sont les fondamentaux nécessaires à des revues de code réussies.

Cet article fait partie de la série :

* Toute ressemblance avec des personnes existantes ou ayant existé n’est peut être pas fortuite. Si vous vous y reconnaissez, nous pouvons peut être vous aider.

Mon équipe ne progresse pas

Au sein d’une équipe que je venais de rejoindre, j’entends un jour Bob râler devant son poste :

“Mais il n’est pas possible Martin, combien de fois a-t-on dit qu’on ne construisait plus les requêtes SQL avec des Strings mais avec des Criteria Hibernate !”

Je suis allé voir Bob pour comprendre ce qu’il se passait. Il m’explique alors qu’il a revu du code écrit par Martin et qu’il a encore dû corriger une requête SQL qui ne répondait pas aux standards.

Moi : “Mais, ils sont écrits quelque part vos standards ?”

Bob : “Non… mais on les connaît, Martin est le seul à ne pas les respecter. Bon en même temps il est arrivé récemment.”

Moi : “Effectivement… Mais j’ai une autre question : tu as dit que tu as dû corriger le code toi même ?”

Bob : “Oui, on fait comme ça, je préfère corriger moi même pour être sûr. Et je ne vais pas aller l’interrompre pour ça.”

Moi : “Et si on allait lui en parler ?”

La revue n’occasionne pas d’échange

Dans cette conversation, pourquoi Martin n’a-t-il pas intégré le standard ?

Relire le code de quelqu’un, c’est l’occasion de lui donner un feedback sur son code, et de l’aider à améliorer ses pratiques. Sans échange avec l’auteur du code, le relecteur a beau détecter les défauts, ceux-ci réapparaîtront inlassablement dans le code futur car l’auteur n’a pas obtenu de retour dessus.

La revue de code doit être préparée par le relecteur, au moyen d’une première relecture pour relever des défauts ou des questions à poser. Ceci permet d’en effectuer une partie en asynchrone, mais le risque est qu’elle se termine sans occasionner cet indispensable échange, idéalement verbal.

L’auteur ne corrige pas les défauts

L’exemple précédent montre aussi un manque de confiance de la part du relecteur envers l’auteur du code. C’est dommage car le meilleur moyen de progresser, d’apprendre et intégrer un standard est de l’appliquer soi même.

Si la revue de code est l’occasion de transmettre du savoir, c’est aussi le cas de l’étape de correction des défauts :

  • il est donc important que ce soit l’auteur qui corrige les défauts relevés.
  • plutôt que le relecteur corrige les défauts par manque de confiance, on peut démarrer une séance de Pair Programming et construire la correction à deux.

Il s’agit aussi de responsabiliser l’auteur du code : corriger les défauts d’un autre développeur peut l’infantiliser, alors que l’aider à les corriger lui même le responsabilise.

Des standards non partagés

L’utilisation de standards de code est une autre pratique essentielle pour parvenir à des revues efficaces. Ces standards sont construits collectivement par l’équipe, et écrits par exemple dans un wiki.

S’ils ne sont pas écrits, on va forcément les oublier ou les réinterpréter, et donc ne pas les assimiler. Ces standards, on les initie dès le début d’un projet, et on les fait évoluer dès que nécessaire, par exemple lors d’une “heure tech” récurrente.

Mes revues se transforment en débats sans fin et autres “trolls”

Pendant la première revue de code collective organisée au sein de l’équipe :

Kent : dis voir Becky, à la ligne 984, ton accolade n’est pas à la ligne, et en plus ta variable $moneyList ne respecte pas la casse standard : ici on utilise du snake_case !

Becky : alors, d’une part il n’y a pas de “casse standard”, je ne suis pas d’accord. Et de toute façon c’est has been ! Si on suivait la norme, on serait en PSR 42 et il n’y aurait pas de question à se poser !

Kent : Mouais il y a des choses bien dedans, mais je ne suis pas d’accord avec leurs règles de nommage !

La discussion dure encore un bon moment, arrêtons-nous ici.

Qu’elles soient au format collectif ou en binôme, les premières revues de code organisées au sein d’une équipe sont souvent “polluées” des débats interminables, généralement sur des points censés être simples.

Afin de couper court à ces débats, il est indispensable d’établir au plus tôt des standards écrits, comme on l’a vu auparavant. Et ceci surtout pour des choses pouvant paraître anodines comme le nommage (quelle norme PSR utilise-t-on en PHP ?) ou des particularités controversées du langage (faut-il utiliser var en C# ?).

Il est peu probable que toute l’équipe soit parfaitement alignée sur ces points. Chez OCTO non plus nous ne sommes pas d’accord sur tout. L’essentiel est de trouver un compromis, et de l’écrire dans les standards.

Malgré cela, on rencontre aussi des aspects non traités par les standards, ou des points plus compliqués qui nécessitent une discussion avant de décider d’une correction.

C’est pour cela que, pour conserver une revue efficace :

  • Quel que soit le format, toute discussion ne concernant pas la détection d’un défaut est reportée après la revue.
  • En revue collective, on ne passe pas plus d’une minute de discussion par défaut trouvé. Un modérateur et un gardien du temps y veillent.

Si un point fait débat, on le note dans une liste « à décider, non standard » : le tout est que le débat n’ait pas lieu au cours de la revue.

Alors, comment réussir vos revues ?

Au travers de ces deux retours d’expériences, nous vous avons déjà donné quelques pistes, mais s’il n’y en avait qu’une à retenir la voici : soyez vigilants en organisant vos premières sessions de revue.

Si elles se passent mal dès le début et que vous ne résolvez pas rapidement les problèmes, vous vous tirez une balle dans le pied. La pratique risque alors d’être abandonnée ou déformée.

Voici les points clés que nous vous proposons :

  • Si votre équipe est novice avec la revue de code, commencez par vous former et privilégiez des sessions collectives, au moins dans un premier temps.
  • Formalisez votre processus de revue de code et veillez à bien le respecter : comment doit-elle se dérouler ? quand ? avec quels supports ?
  • Faites évoluer votre pratique de la revue au cours de vos rétrospectives.
  • Appuyez-vous sur des standards que vous faites évoluer et constituez des checklists.

La semaine prochaine, nous vous proposerons un nouvel épisode riche en rebondissements : Malgré la mise en place des revues, on croule toujours sous les bugs.

En attendant, n’hésitez pas à partager vos retours d’expérience via les commentaires, qu’ils se soient bien ou mal passés !

Merci aux OCTOs de la tribu Software Craftsmanship pour leurs précieuses contributions.

2 commentaires sur “Comment rater vos revues de code ? – Épisode 1”

  • Excellent article ! Merci. Je tiens à préciser que j'apprécie la lecture de vos articles : techniques, gestion agile, sur différents sujets et toujours abordables. Je ne connaissais pas le format "revue de code collective". Je trouve que c'est une bonne idée. Mais clairement susceptible aux trolls, disgressions et autres discussions techniques interminables, contre-productives et hors-sujets. Comme vous le soulignez, il me paraît indispensable de bien préparer la session, d'avoir un modérateur et de respecter des règles simples prises en compte par tous. La mise en place de se format doit être plus facile si quelqu'un a déjà tenté l'expérience et vient déjà avec des bonnes/mauvaises pratiques pour bien guider les 1ères sessions. Pour ma part, nous faisons beaucoup de revue de code par les pairs. La plupart du temps via un outil en ligne, et parfois en mode "pair-programming". Le mec s'assoit à côté et on discute devant les lignes de code. Les lignes qui suivent est un point de vue personnel sur cette expérience. Contexte : une quinzaine de collègues, des équipes de dev de 1 à 3 sur des projets courts, souvent 2-3 mois. Technos des différentes équipes : beaucoup de Python, un peu de C++, Javascript et SQL. La procédure : chaque patch de chaque projet est soumis à la revue. Un développeur est tiré au sort parmis tous les développeurs (et non pas un dév du projet). On ne peut évidemment par être relecteur de son propre patch, cela va de soit. Avantage : on lit du code d'autres projets, cibles clientes différentes, fonctionnel différent, technos différentes. On est dans le cas du partage de compétences et de l'apprentissage par la lecture de code. "Tiens, je ne connaissais pas cette fonctionnalité en Python", etc. Inconvénient : on lit du code d'autres projets, cibles clients différentes, fonctionnel différent, [...]. Bref, l'inconvénient de son avantage. On n'est pas forcément compétent pour relire un code Javascript si on fait quotidiennement du C++/Python. À part un peu de syntax ou faire des remarques sur les typos, je ne trouve pas cela très pertinent. On faisait nos remarques sur le patch en rédigeant des commentaires. Cela pouvait être un processus long comparé à un échange à l'oral pour avoir quelque chose de rapidement intelligible. "Hey Bob, j'ai pas compris ton commentaire concernant mon patch". Au final, on en revient souvent à discuter. L'avantage est que tout cela est tracé et historisé. Si quelqu'un passe par là, il peut se taper l'histoire des commentaires du patch. En vrai, cela n'arrive pas si souvent. Bref, un subtil mélange des deux pourrait faire l'affaire. Règles, conventions et bonnes pratiques de la revue plus formalisées auraient été une très bonne chose, pour peu que ces dernières ne soient pas trop libres d'interprétations. Il y avait aussi un syndrome parmis les développeurs de manque d'investissement et d'intérêt pour la revue de code. Les revues étaient d'inégales qualités. Certains passaient ça "à la va-vite" : une remarque sur de la syntaxe sans conséquence, une typo dans le message de commit et c'est tout. Difficile de savoir si cela était était dû aux trop nombreux projets différents revus par chaque ingénieur, ou l'outil de revue, ou un savant mélange de tout cela et d'un peu d'autres choses... Pour finir, nous manquions malheureusement de rétrospectives sur notre système de revue. Et les changements pour l'améliorer étaient lents à arriver.
  • Bonjour jazzydag, merci pour ce retour d'expérience détaillé, c'est très intéressant !
    1. Laisser un commentaire

      Votre adresse de messagerie ne sera pas publiée. Les champs obligatoires sont indiqués avec *


      Ce formulaire est protégé par Google Recaptcha