Pratiques pour des revues de code réussies

La revue de code c’est le fait de poser une autre paire d’yeux (au moins) sur le code avant de l’envoyer en production. C’est un outil peu coûteux et qui rapporte beaucoup, ça vaut le coup de faire ça bien.

Sans standard, aucune valeur

Un des principes fondateurs du lean et de l’agile, c’est la qualité “built in”. C’est à dire qu’on n’attend pas la fin pour vérifier que ça marche et corriger les erreurs, on apprend au fur et à mesure comment faire bien du premier coup.

Dans cet esprit, le but de la revue de code n’est pas de corriger des erreurs, mais d’apprendre à ne plus en faire.

Les pièges classiques

Il y a pas mal de pièges dans lesquels tombent les équipes qui débutent dans les revues de code :

• La revue asynchrone.

Si vous voulez progresser rapidement en tant qu’équipe, alors chaque revue doit être un moment d’échange, de discussion, entre plusieurs développeurs. Si vous faites vos revues de manière asynchrone, dans un outil, ce n'est pas un problème en soi. Mais vous risquez de vous limiter à mettre vos remarques dans cet outil sans en discuter de vive voix. Sans la discussion, vous irez beaucoup plus lentement et vous risquez de créer des tensions qui empêcheront l’apprentissage.

• L'interprétation artistique.

On voit généralement quand il n’y a pas une norme claire et partagée. Chacun aura une vision légèrement différente de ce qu’est un code propre et les revues se feront suivant la sensibilité de chacun, parfois de manière contradictoire.

• La bataille d’ego.

La revue est un moyen de s’améliorer ensemble. Mais si on perd de vue cet objectif, on peut tomber dans la critique et risquer de faire un retour maladroit, condescendant ou agressif. Si cet état d’esprit perdure, tous les membres de l’équipe vont se crisper et la revue sera un point focal de tensions.

• Le superviseur.

Dans certaines équipes, seul le tech lead ou l’expert technique fait les revues pour tous les autres. En plus d’être un goulot d’étranglement, ce qui va obligatoirement entraîner des revues asynchrones et du stock, on crée beaucoup plus facilement une atmosphère propice à la bataille d’ego.

• Les erreurs récurrentes.

Enfin, un signe simple et clair que vos revues n’ont pas l’effet escompté est l’erreur récurrente. Si vous constatez que vous faites encore et encore les mêmes types de retours, c’est que l’apprentissage n’a pas lieu.

La pire façon de faire des revues que j’ai rencontrée dans ma carrière était dans un service informatique où les développeurs devaient soumettre leur code terminé à un service à part composé d’experts techniques. On entendait dans l’open space ces experts rigoler entre eux et se moquer du code qui leur était envoyé. Leurs remarques dans l’outil étaient violentes et condescendantes. En plus, les experts avaient chacun leur définition d’un code valide et il n’était pas rare d’avoirs des retours contradictoires en soumettant le même code plusieurs fois.

Je vous laisse imaginer l’efficacité de cette organisation.

Le standard à la rescousse

Le meilleur moyen que je connaisse pour se sortir de toutes ces situations et capter un maximum de valeur des revues, c’est d’adopter un standard de code au sens lean.

Le standard, c’est la meilleure façon de faire connue à ce jour par l’équipe.

Plus l’équipe aura participé à la construction de ce standard, plus elle y adhérera. Il sera leur consensus, adapté à leurs connaissances actuelles. Le standard doit être respecté, mais n’est pas non plus figé : l’équipe apprend de manière continue et découvrira de meilleures façons de faire au fur et à mesure.

Pour ces raisons, les standards imposés aux équipes par des cellules d’architectes transverses sont rarement vus positivement. Dans ces cas là, le nombre de retours est systématiquement élevé.

Un standard écrit, partagé et accepté par l’équipe a de nombreux avantages :

• Pas d’interprétation possible et pas de problème d’ego : soit le code est au standard, soit non.
• Pas de goulot d’étranglement : tout le monde peut faire des revues puisqu’il s’agit de comparer du code au standard. Le rôle du tech lead n’est plus de s’assurer que le code de chacun est bon, mais que chacun est capable de faire la différence entre du bon code et du mauvais code.
• L’apprentissage est plus facile : à force d’avoir des retours sur une même règle du standard, on finit rapidement par l’intégrer et y faire plus attention. Le tech lead est là aussi pour faire des focus sur les règles qui posent le plus de problèmes à l’équipe.
• Enfin, on facilite aussi l’intégration des nouveaux développeurs. Un standard écrit lui permet de faire un premier commit acceptable et de s’éviter le stress d’un volume anormal de retours quand on est en période d’essai.

Le standard et les revues sont deux faces d’une même pièce : le standard permet de faire les revues de code, et les revues de code permettent de construire et d’alimenter le standard.

La revue formelle

Ce que j’appelle la revue formelle, c’est une revue avec toute l’équipe, dans une salle avec le code projeté pour que tout le monde puisse travailler sur le même sujet.

Préparer la réunion et choisir le sujet

Une revue formelle est le meilleur moyen de partager un concept ou un focus avec toute l’équipe.

On peut vouloir faire des revues formelles pour atteindre les objectifs suivants :

• Faire évoluer le standard.
• Revenir ensemble sur une règle qui pose problème à l’équipe.
• Passer au peigne fin une partie critique du code pour s’assurer qu’il n’y a pas d’erreurs ni de problème de design.

C’est au tech lead d’organiser ces revues. Il peut choisir d’en faire régulièrement ou seulement si besoin.

Il est conseillé de préparer le sujet et de bien choisir les morceaux de codes qui seront évalués. Il faut bien se focaliser sur un petit nombre de sujets pour éviter de se perdre et de ne rien accomplir pendant la séance.

Profitez de ces réunions pour inviter des experts extérieurs à l’équipe (architecte, testeur, expert sécurité ou performance, ops) qui peuvent vous aider sur le sujet du jour.

Il ne faut pas hésiter à imprimer les règles et le code sur lesquels on va se focaliser. Ça permettra de prendre des notes et de garder l’apprentissage sur un support lisible et facilement conservable.

Pour un focus sur des règles du standard, 1h-1h30 par règle suffisent. Pour une inspection de groupe, prenez vous la journée : si le code est suffisamment critique pour faire ce genre d’atelier, alors il faut se donner le temps de faire ça bien.

Le déroulement de la revue

J’aime bien commencer par 5-10 minutes de présentation du sujet et rappel théorique si besoin. Ensuite, je sépare cette réunion en cycles de deux étapes : la lecture et la modification.

Pour chaque morceau de code qu’on va étudier, je laisse un temps approprié pour que chacun le lise, le comprenne et prépare ses annotations. C’est là où le code imprimé pour chacun est utile.

Ensuite, on discute du code et on fait les modifications nécessaires. C’est plus facile d’apprendre avec les doigts, en apprenant à remettre du code aux normes.

Ma préférence va pour un mode driver/navigators tournant où une personne va au clavier mais n’a le droit d’écrire que ce que dit le groupe. C’est donc au groupe de discuter et de se mettre d’accord sur ce qu’il faut changer. Toutes les 10 minutes on tourne.

Dans le cas des points complexes (design ou sécu par exemple) c’est bien de prévoir un tableau et des marqueurs.

Enfin, pour éviter de trop dériver, c’est bien de garder un oeil sur le temps et de timeboxer les inspections.

Le suivi après la revue

La revue en elle même apporte beaucoup à l’équipe mais le gros de la valeur se trouve après.

Après avoir étudié une ou plusieurs règles du standard en revue formelle, on doit constater une baisse du volume de retours sur ces règles lors de revues au fil de l’eau. Si ce n’est pas le cas pour tout le monde, le tech lead doit prendre un peu de temps pour binomer afin d’y revenir plus en détail.

Dans le cas d’une revue ayant pour but de passer au peigne fin du code pour repérer des bugs ou des erreurs, il faut prendre ensuite un moment pour les analyser et trouver le moyen de faire en sorte que ce genre d’erreurs ne se reproduisent plus.

Les revues au fil de l’eau

Plus la revue de code a lieu tôt, plus elle sera efficace. Les formes les plus avancées sont le pair programming et le mob programming mais on verra ça dans d’autres articles.

Plus facile à mettre en place : la revue au fil de l’eau en fin de développement de story.

Quand faire la revue au fil de l’eau ?

Il y a un équilibre à trouver entre faire la revue rapidement et ne pas interrompre n’importe quand les autres développeurs en plein milieu de leurs tâches :

• En découpant agressivement les stories.

Tout est plus facile quand les stories sont petites. La revue de code n’y échappe pas : si les développeurs peuvent commencer et terminer plusieurs stories dans la journée, alors il y aura de nombreux créneaux de libres pour faire des revues sans interrompre quelqu’un. Et comme les stories sont petites, les revues sont rapides.

• En mettant une limite d’encours.

Si les stories ne sont pas assez petites, alors il n’y aura pas toujours quelqu’un de libre pour faire une revue immédiatement, et elles vont prendre du temps. Donc un stock va se créer. Pour éviter que toutes les stories s’empilent et que les revues soient faites en panique à la fin du sprint, il vaut mieux mettre une limite d’encours pour forcer les développeurs à faire une revue avant de prendre une nouvelle story.

• En créant des créneaux réservés.

Enfin, si les stories sont vraiment trop grosses, le délai entre la fin d’un développement et sa revue risque d’être trop grand. Donc pour éviter les interruptions, vous pouvez aménager des créneaux quotidiens réservés. Typiquement juste après le standup ou juste après la pause dej. Comme ça, avant de se remettre dans sa tâche principale, chacun va se focaliser sur les revues. On évite le context switching, et au pire le délai est d’une demi journée.

Les bonnes pratiques

Avec le travail fait sur le standard et avec les revues formelles, l’apprentissage pendant les revues au fil de l’eau va être facilité. Chacune de ces petites revues va permettre à l’équipe de réduire progressivement le nombre de retours.

L’objectif, c’est d’arriver à un idéal où, sauf pour les cas où on découvre un bug, chaque remarque dans la revue doit être un renvoi au standard. Vous devriez être capable de pointer une partie du code et de dire “violation de la règle 12bis”. Ce renforcement fait rentrer le standard, et ça évite des discussions ou des explications.

Bien sûr, tout n’est pas immédiatement aussi clair : vos premières revues donneront lieu à beaucoup de discussions et interprétations. C’est un travail au long cours : ces discussions devront permettre d’affiner le standard.

La détection des violations les plus évidentes, qui ne laissent pas de place à l’interprétation, devra être automatisées dans un linter. Par exemple, on peut lever automatiquement une erreur si une méthode est trop longue ou si un seuil de complexité est passé.

Le rôle du tech lead n’est donc pas de revoir le code de chacun, mais de s’assurer que toute l’équipe apprend et affine son standard au fur et à mesure afin de réduire de manière durable le nombre de retours et donc de corrections.

Bonus : je met quoi dans mon standard de code ?

Le but du standard c’est de permettre à chacun de faire la différence entre du bon code et du mauvais code.

Le bon code c’est un code lisible et facilement changeable. Donc tout ce qui entrave la compréhension ou la modification est à surveiller.

Lisible

Une méthode doit être compréhensible entièrement en moins de 5 minutes. Si on est obligé de déchiffrer, de prendre du temps pour comprendre, on risque de rater des éléments et on complique la modification.

Pour faciliter la lecture on se mettra des règles sur le nommage, les niveaux d’abstraction, la complexité, la cohésion, les dépendances, la documentation et les tests.

Facilement changeable

La facilité à changer le code est un problème de premier ordre. Plus votre code est rigide, plus vous aurez de difficultés à suivre un rythme itératif et incrémental.

Pour faciliter la modification, on fera attention au design, à la séparation entre le domaine et la tuyauterie, à la duplication, au code mort, au code non couvert par les tests, aux tests instables etc..

Sûreté et sécurité

On peut aussi mettre dans le standard des règles pour se forcer à faire plus attention aux problèmes de sécurité, de performance, de stabilité ou d’accessibilité. Ces domaines sont trop souvent une arrière pensée, une story à faire à la fin quand tout le reste est fini, ce qui augment le risque de se tromper ou de ne pas finir.

Il faut apprendre à être bon du premier coup sur ces sujet aussi.