Ce que je regarde vraiment dans une revue de code

Je relis du code tous les jours. Au début, je commentais tout ce que je voyais : un nommage approximatif, une accolade mal placée, une fonction un peu longue. Les revues duraient longtemps, agaçaient tout le monde, et laissaient passer les vrais problèmes.

J’ai fini par comprendre que la revue de code est un temps rare, et qu’il faut le dépenser là où la machine ne peut rien.

Ce que la machine doit faire à ma place

Avant même d’ouvrir une revue, tout ce qui est mécanique doit déjà être traité automatiquement : mise en forme, ordre des imports, longueur des lignes, conventions de nommage, complexité excessive, code mort.

Ce n’est pas un détail d’organisation. Tant que ces sujets peuvent apparaître en revue, ils occupent l’espace — et pire, ils créent une asymétrie désagréable où le relecteur a l’air de juger le style de quelqu’un. Un outil qui dit la même chose ne vexe personne.

Ma règle : si un commentaire pourrait être écrit par un outil, il ne doit pas être écrit par un humain.

1. Les cas d’erreur

C’est la première chose que je cherche, et de loin la plus rentable.

Le chemin nominal est presque toujours correct : c’est celui que le développeur a exécuté en testant. Ce qui manque, ce sont les autres.

Concrètement je cherche : que se passe-t-il si l’appel externe échoue, ou pire, s’il met trente secondes à répondre ? Si la liste est vide ? Si la valeur est absente plutôt qu’à zéro ? Si deux utilisateurs font la même action en même temps ? Si l’opération échoue au milieu — que reste-t-il en base ?

La question que je pose le plus souvent en revue tient en cinq mots : « et si ça échoue ici ? » Elle trouve plus de bugs que tout le reste réuni.

2. Les frontières

Un module qui accède directement aux données d’un autre, un composant d’interface qui contient une règle de gestion, une couche technique qui connaît le métier : ce sont des fuites de frontières.

Elles ne cassent rien aujourd’hui. Elles rendent chaque modification future plus coûteuse, parce que le changement se propage à des endroits qu’on n’avait pas prévus.

C’est le type de problème qu’il faut absolument attraper en revue, parce qu’il est bon marché à corriger maintenant et cher à corriger dans six mois. Une fois que trois autres bouts de code se sont appuyés sur la fuite, on ne la corrige plus.

3. La lisibilité pour quelqu’un qui découvre

Je ne me demande pas si le code est élégant. Je me demande : est-ce qu’un développeur qui arrive dans six mois comprendra pourquoi c’est écrit comme ça ?

Ce qui compte n’est pas ce que fait le code — on le lit — mais pourquoi il le fait ainsi. Un contournement d’une limitation d’API, une règle métier contre-intuitive, un ordre d’opérations qui a une raison : sans commentaire, ces choix seront « nettoyés » par quelqu’un plus tard, et le bug reviendra.

Je demande donc souvent des commentaires, mais uniquement sur le pourquoi. Un commentaire qui paraphrase le code est du bruit ; un commentaire qui explique une décision vaut de l’or.

Photo : Christina Morillo — licence CC0.

4. La testabilité

Je ne compte pas les tests, je regarde si le code peut être testé.

Une fonction qui appelle directement l’horloge système, le réseau ou la base est difficile à tester. La même fonction, avec ces dépendances passées en paramètre, se teste en trois lignes.

Quand je vois un test compliqué, avec beaucoup de préparation, je ne commente pas le test : je commente le code testé. Un test difficile à écrire est presque toujours le symptôme d’une conception trop couplée.

Photo : Christina Morillo — licence CC0.

Les cinq commentaires que j’évite

« Je n’aurais pas fait comme ça. » Sans raison explicite, c’est une préférence, pas une revue. Si je ne sais pas expliquer pourquoi ma version est meilleure, je laisse passer.

Les débats de style. Voir plus haut : l’outil s’en charge, ou personne.

Les réécritures complètes en commentaire. Si le désaccord est structurel, une conversation de dix minutes vaut mieux que quarante lignes de commentaires. La revue écrite est un mauvais médium pour les désaccords de fond.

Les remarques sans priorité. Un relecteur qui laisse quinze commentaires de même poids oblige l’auteur à deviner lesquels sont bloquants. J’utilise trois niveaux explicites : bloquant, à corriger, suggestion.

Le silence. Approuver sans rien dire sur une modification importante n’est pas une revue, c’est un tampon. S’il n’y a vraiment rien à dire, je le dis — et j’explique ce que j’ai vérifié.

Comment je formule un retour

Le fond compte, la forme détermine si le fond sera entendu.

Poser une question plutôt qu’affirmer. « Que se passe-t-il si la liste est vide ? » plutôt que « tu as oublié le cas de la liste vide ». Ce n’est pas de la politesse : parfois le cas est géré ailleurs, et je l’apprends au lieu de me tromper en public.

Dire ce qui va bien. Un découpage propre, un nom bien choisi, un cas limite anticipé : je le signale. Une revue qui ne contient que des reproches use la personne qui la reçoit, et l’on finit par ouvrir ses revues à reculons.

Distinguer ce qui bloque de ce qui n’engage que moi. Je préfixe : bloquant pour ce qui doit changer avant fusion, à corriger pour ce qui peut passer en suivi, suggestion pour ce que l’auteur est libre d’ignorer. Sans ces trois niveaux, chaque commentaire pèse pareil et l’auteur perd du temps à deviner.

Basculer à l’oral au bout de trois allers-retours. Si un point n’est pas réglé après trois échanges écrits, c’est qu’il y a un désaccord de fond ou un malentendu. Dix minutes de conversation valent alors mieux que trois jours de commentaires.

Le cas de la revue faite par quelqu’un de moins expérimenté

On considère souvent qu’une revue va du senior vers le junior. C’est dommage, et je fais l’inverse aussi souvent que possible.

Un développeur qui débute pose les questions que personne n’ose poser : pourquoi cette classe s’appelle ainsi, pourquoi ce détour, pourquoi ce fichier fait huit cents lignes. Ces questions sont exactement celles qui révèlent la dette de compréhension.

Et pour lui, relire du code est le moyen le plus rapide d’apprendre une base : bien plus efficace que la lire seul, parce qu’il y a quelqu’un pour répondre.

La taille, encore

Une remarque qui n’a l’air de rien et qui change tout : une revue de deux cents lignes est sérieuse, une revue de deux mille est un tampon.

Au-delà d’un certain volume, le relecteur ne peut plus tenir tout le contexte en tête. Il survole, approuve, et le bénéfice disparaît. Depuis que nous générons plus de code, ce point est devenu critique chez nous : nous avons réduit la taille des lots plutôt que d’accepter des revues plus grosses.

Si une modification ne peut pas être découpée, c’est souvent qu’elle mélange plusieurs intentions — et c’est déjà, en soi, un retour de revue utile.