Dev. Junior: C’est quoi la différence entre une bonne et une mauvaise review ?

Dev. Senior: Ah je l’attendais cette question, je l’attendais, je savais que t’allais la poser, j’en étais sur … alors Junior tu vois, déjà la mauvaise review: t’as fait ton code, t’as push ta PR, ok … t’as un retour, on te suggère une amélioration sur le code … voilà … et puis t’as la bonne review: t’as fait ton code, t’as push ta PR, ok … et là t’as un retour, on te suggère une amélioration sur ton code … mais là Junior, par contre, c’est une bonne review … c’est pas pareil quoi …

Dev. Junior: ¯\_(ツ)_/¯

Explication limpide (ou pas) de la différence entre une bonne et une mauvaise review

Si cet extrait d’une discussion entre deux développeurs à la “machine à café” vous évoque quelque chose, alors la suite de cet article est faite pour vous : en effet, c’est quoi une Bonne et une Mauvaise review ?

Déjà, c’est quoi une review ?

La review, ou revue de code en français dans le texte, est une étape présente dans de nombreuses entreprises technologiques permettant de s’assurer que le code est conforme à ce qui est attendu, de relever des problématiques potentielles, ainsi que de suggérer des optimisations diverses avant que le code n’arrive en production, tout ceci dans le but d’éviter de devoir sortir des patchs après coup ou d’augmenter la dette technique.

C’est une pratique inspirée des publications scientifiques et médicales, où un article ne peut être publié sans avoir été relu par des pairs, les données vérifiées, et les expériences reproduites. Cela permet de conserver la rigueur scientifique requise en évitant les méthodologies douteuses et la course à la publication.

Tout commence par une Pull Request

Pour bien comprendre le fonctionnement d’une review, et donc comment la réussir ou la rater, il est essentiel de bien comprendre à quel moment du cycle de développement cette étape arrive.

Chez Deepki, comme dans de nombreuses entreprises informatiques, nous utilisons un système de gestion de version. Quand nous commençons à travailler sur une fonctionnalité, nous créons une branche qui dérive de la branche principale. Nous ajoutons notre code dans cette branche dédiée, et une fois notre travail terminé, nous souhaitons l’intégrer dans la branche principale pour que les modifications puissent être accessibles à tout le monde. C’est à ce moment, avant l’intégration dans la branche principale, que le processus de review arrive, et c’est sur ce moment que nous allons nous concentrer dans la suite de cet article.

Exemple de branche Git

Qu’attendons-nous durant une review ?

En tant que développeur, nous attendons d’une review qu’elle agisse principalement comme un filet de sécurité. Après tout, nous sommes des humains, et nous sommes faillibles. Deux paires d’yeux valent mieux qu’une. Quand on écrit du code, ce dernier se doit de remplir un certain nombres d’objectifs: fonctionnel, sécurité, lisibilité, élégance … Sans pour autant chercher la perfection, une review efficace peut nous permettre de remplir ces objectifs.

Quand nous pouvons travailler de manière synchrone, que ce soit physiquement ou à distance, une bonne façon de s’assurer un code de qualité consiste à faire du “pair programming” (ou du “mob programming”): cela nous permet d’échanger/commenter en direct le code produit, de partager la connaissance, de suggérer des améliorations, et de repérer rapidement les problèmes/failles éventuelles. Mais toutes les équipes n’ont pas le luxe de travailler de manière synchrone, par exemple pour des raisons physiques (équipe répartie sur plusieurs fuseaux horaires) ou tout simplement pour des raisons d’indisponibilités.

Pour ne pas bloquer le développement, les équipes doivent donc se reposer sur un processus de travail asynchrone:

  • le développeur original pousse son code dans une Pull Request
  • on attribue la relecture de cette PR à un autre développeur
  • quand il a le temps, ce développeur lit le code, et annote ses commentaires
  • l’auteur original lit ces commentaires, et les intègre s’il le juge nécessaire
  • quand tout le monde est d’accord, le développeur effectuant la review valide le code, et ce dernier peut-être intégré dans la branche principale.

Mais cette étape de review, mal réalisée, peut rapidement devenir contre-productive. Regardons dans le détail comment rater sa review.

Case 15 du Calendrier de l'Avent 2023 MonkeyUser

Tous les conseils pour rater sa review (aka “Make Our Code a Big Pile of Crap”)

Faire manuellement ce que vous pouvez automatiser

Le temps est un bien précieux, qui n’est pas extensible à l’infini. La façon dont vous utilisez ce temps en dit beaucoup sur vous. Certains reviewers vont se contenter d’une review superficielle, par exemple en vérifiant que les règles et conventions sont respectées, et ne vont pas plus loin. Dépenser son temps de manière futile et triviale est une bien mauvaise façon d’utiliser ce temps, surtout quand toutes ces tâches sont facilement automatisables. Les langages de développement modernes disposent généralement au moins d’un linter, d’un ensemble de règles d’écriture de code, d’un outil d’analyse statique … Out of the box, ils vous permettront d’automatiser toutes ces tâches futiles.

Une Pull Request trop grande

« We're gonna need a bigger review team »

Qui n’a jamais ressenti un début de migraine devant la review d’une Pull Request comportant +1000 lignes modifiées dans ~250 fichiers différents ? D’autant plus si la description de la PR se contente d’un laconique “voir ticket #XXX” en guise d’explication. Une mauvaise review commence souvent par une mauvaise Pull Request. Alors me direz-vous: “C’est quoi une bonne Pull Request ?”. Cela mériterait un article à part entière. Ceci dit, un point commun que l’on retrouve fréquemment dans d’autres articles (1, 2, 3 …) est qu’une bonne PR doit être “petite, avec de grands changements”. En effet, une Pull Request trop importante n’incite pas le reviewer à rentrer dans les détails, et ce dernier aura trop souvent tendance à se contenter de retours triviaux, en cherchant plutôt des suggestions “visuelles” comme du naming, en parcourant d’un oeil les fichiers sans rentrer dans le détail algorithmique. Rappelez-vous: le temps du reviewer n’est pas extensible à l’infini.

Prendre la review pour un examen de fin d’études

Une review est l’endroit où, collectivement, vous allez améliorer le code écrit par un développeur. Ce n’est ni l’endroit, ni le moment, pour vérifier les compétences de ce développeur en lui faisant passer un examen de fin d’études. Surtout si ce quizz n’est là que pour flatter votre ego en démontrant la supériorité de vos connaissances sur celles de l’auteur original. Mieux vaut expliquer, avec de la documentation en appui de votre argumentation, pourquoi vous estimez que votre suggestion est meilleure que le code original, plutôt que de laisser une liste de propositions sous forme de quizz, sans aucune autre explication. Non seulement, en facilitant la compréhension de votre review, vous en réduirez le temps, mais vous gagnerez également un intérêt pour vos futurs reviews.

Ne pas s’impliquer

Quand le reviewer dicte son idée depuis sa tour d’ivoire, sans avoir au préalable ne serait-ce que testé son idée sur le code en question, il délègue au développeur original la responsabilité à la fois de tester la faisabilité de sa suggestion ET de l’implémenter … Si l’idée fonctionne, le crédit en reviendra au reviewer; dans le cas contraire, l’échec devra être supporté par le développeur, qui aura perdu du temps à implémenter cette idée …

Code Quality Measurement: WTFs/minute

Bref, nous venons de voir qu’il peut-être facile de rater sa review … Regardons maintenant comment la réussir (ou en tout cas mettre un maximum de chances de son côté pour qu’elle soit une réussite).

Tous les conseils pour réussir sa review (aka “Make Our Code Great Again”)

Automatiser tout ce qui peut l’être

Comme on a vu au début: pour éviter au reviewer de perdre son temps sur des points futiles, automatiser tout ce qui peut l’être … Les conventions de nommage (snake_case, camelCase, PascalCase, kebab-case …), les retours à la ligne, les simple vs double guillemets … Il y’a suffisamment d’outils existants dans quasiment tous les langages pour ne plus s’attarder à corriger ce genre de détail. Toutes les automatisations (chaque vérification de standard) que vous ferez sont autant de temps que vous pourrez passer sur des remarques plus intéressantes.

Eviter le toxique, le superficiel, le négatif

Une review n’est pas un combat entre le développeur et le reviewer: votre travail est de créer de la valeur pour le produit (et donc le client). Même une mauvaise review doit être considérée comme étant faite pour améliorer le produit. Ne cherchez pas la perfection dans votre review (there is no such thing as “perfect” code, there is only better code). Si vous souhaitez faire remonter des suggestions non essentielles dans votre review, préfixez les par exemple par Nitpick:. Cela indiquera à l’auteur de la Pull Request qu’il peut passer cette suggestion s’il le souhaite.

On évite de mettre la pression à un reviewer pour accepter une PR: si cela prend du temps, c’est que ce n’est pas si trivial que ça, ou que votre PR n’est pas aussi facile à comprendre. Pourquoi ne pas proposer de faire du pair reviewing dans ce cas ? Concentrez-vous sur l’utile, et n’oubliez pas qu’une review ce n’est pas que de la critique, c’est aussi savoir remercier un refactoring, ou mettre en avant l’utilisation particulière d’une fonction … Un petit “Bravo”, “Good Job”, “Nice Work” est toujours apprécié.

Débattre de design patterns, architecture, code quality, code structure, code readibility …

C’est le bon moment pour aborder ces questions, sans évidemment remettre en question ce qui a été collectivement décidé en amont, ni les règles et standard définis dans votre équipe (Collaborer à plusieurs, c’est aussi du code bien formatté). Suggérez les changements et améliorations que vous avez testés dans la branche sur votre ordinateur. Plutôt que de dire “tu devrais utiliser cette fonction”, fournissez en plus un lien vers la documentation de cette fonction: tout ce qui fera gagner du temps à l’auteur de la PR sera du temps gagné (rappel: le temps n’est pas extensible à l’infini). Dans une Pull Request de qualité, le développeur aura pris le temps d’expliquer ses choix dans la description de cette dernière. Si ce n’est pas le cas, posez des questions pour demander le contexte. En tant que reviewer, vous n’avez pas forcèment le contexte global.

Une belle PR = une opportunité de belle review

Rien que ce point mériterait un article à part entière. Une belle PR, bien présentée, bien découpée en petits commits atomiques, se limitant à son scope, incitera le reviewer à faire une belle relecture. Rappelez-vous: une PR n’est en soit qu’un Work In Progress tant que la revue n’est pas terminée. C’est un travail d’équipe pour aboutir à une décision consensuelle. Un titre expliquant le problème/scope général, une description expliquant le contexte et les choix techniques, aideront le développeur à ne pas perdre son temps à comprendre votre ticket. La taille idéale d’une PR (son différentiel en nombre de lignes) ne devrait pas dépasser ce que vous pouvez comprendre en une heure. En fonction des développeurs, de leur expérience, de leur connaissance de la pile technique, du langage utilisé … cette “taille idéale” peut varier entre 100 et 250 lignes. Si votre PR fait plus, vous devriez probablement la découper en plusieurs petites PR, en prenant soin de faire ce découpage intelligement, et de bien transmettre le contexte global.

Benchmark temps/taille de PR

Dans ce graphique issus de l’article Optimal pull request size de Blaine Osepchuk, ce dernier a demandé à son équipe de noter le temps passé sur chaque review pendant un mois. Il a ensuite intégré le nombre de lignes de code de chacune des reviews pour en sortir ce graphique, et cette conclusion: “Nous devons limiter nos pull requests à 200 lignes de code changées pour avoir 90% de chances de faire la review en une heure ou moins”.

Faites vos PR comme vous voudriez lire celle des autres. Lead by example.

En résumé

Votre review sera réussie si (non-exhaustif et non cumulatif):

  • vous avez identifié un problème (sécurité, performance, design …)
  • votre équipe a appris quelque chose pendant la review
  • il y’a eu un débat sur la nécessité d’une portion de code (duplication etc …)

Jugez votre review: fournit-elle plus d’infos qu’une simple recherche Google ?

Ne vous concentrez pas que sur le négatif, mettez aussi en avant le positif, les pépites cachées dans le code.

Vous aurez gagné à chaque fois que votre review aura permis l’amélioration du code.

Donc lors de votre prochaine review, posez-vous la question: ma review a-t-elle permis d’améliorer le produit ?

Sources

Voici quelques articles qui m’ont inspirés pour améliorer mes reviews :

Et en prime, quelques conseils pour faire de belles Pull Requests :