Comment faire une bonne revue de code

Je travaille beaucoup avec Git et des outils tels que Bitbucket, Gitlab, Github, VSTS et autres… Mais, malgré ces outils et la bonne volonté des équipes suivant avec rigueur le Gitflow, il est très difficile d'empêcher les bugs de passer dans la base stable de code. En rétrospective de sprint avec mon équipe, le coupable a été pointé du doigt : la revue de code (ou code review).

La revue de code est un grand sujet chez les développeurs. Elle prend plusieurs formes, mais je vais parler ici de celle que j'utilise quotidiennement. Tous les jours je passe bien 1 à 2 heures à faire de la revue de code pour valider les pull requests (PR) de mes collègues.

Voici donc les quelques conseils dont je souhaite laisser une trace sur ce blog.

Automatisez les vérifications du styleguide

Nous avons automatisé le plus de tâches possible pour libérer les développeurs et simplifier le travail lors de la revue de code. Et, je pense réellement que ce n'est pas un luxe.

Je ne compte plus le nombre de pull requests où le nombre de commentaires étaient supérieur à 20 mais ne servaient qu'à préciser qu'il manquait un espace entre l'accolade ouvrante d'un objet littéral et la première clé de ce même objet. Certes c'est important, les projets doivent rester consistant, cohérent comme s'ils avaient été écrit par un seul et même développeur mais pourquoi devrait-on utiliser les immenses capacités de réflexion d'un cerveau humain pour faire des vérifications qu'une machine peut très bien faire toute seule?

Malheureusement, ce genre de commentaire, bien qu'utile, à la capacité d'énerver et de vexer l'auteur du code incriminé. Pourquoi ? Parce que justement, c'est un autre humain qui fait le commentaire. Lorsqu'il s'agit d'une machine, cela se passe généralement mieux : la PR est bloquée tant que le styleguide de l'équipe/du projet n'est pas respecté, l'auteur ne peut s'en prendre qu'à lui-même de ne pas avoir respecté une règle clairement définie et les autres membres de l'équipe peuvent utiliser leurs cerveaux pour faire de bonnes recommandations.

Et en parlant de recommandations et de conseils, j'ai remarqué que lorsque je devais faire attention à la mise en forme du code pendant que je faisais ma revue, il m'arrivait plus souvent de passer à côté de problèmes de conception bien plus importants et intéressants à relever.

Automatiser l'exécution des tests

Les tests unitaires et autres servent potentiellement à plein de choses. Je m'en sers, par exemple, pour comprendre comment fonctionne une lib externe. J'utilise aussi les tests unitaires pour réfléchir à une problématique : je pose par écrit ce que mon code doit faire avant même de commencer à réfléchir à comment il va le faire. Mais, une des choses pour lesquels les tests unitaire sont vraiment utiles, c'est éviter la régression.

Bien-sûr, encore faut-il exécuter régulièrement ces tests. Le développeur consciencieux l'aura forcément fait sur sa machine avant de pousser son code. Mais il nous arrive à tous d'oublier des fois et puis, qui nous dit que le code qui passe les tests unitaires sur une machine où un développeur trafficote tous les jours passera aussi en environnement serveur automatisé ?

Je connais pas mal d'équipes qui exécutent les tests unitaires sur develop ou master dès qu'une PR est acceptée. C'est très bien ! Mais, ne serait-il pas mieux de savoir, avant d'accepter une PR, si le code produit et ajouté ne va pas passer les tests unitaires (et donc, s'ils sont bien écrit, casser quelque chose) ?

Si la pull request est bloquée lorsqu'un test fail, l'auteur de la PR devra la corriger pour que sa pull request soit acceptée. Le problème sera donc corrigé et le code rendu « stable » avant d'être intégré sur les branches stables du projet.

C'est d'autant plus important si le projet est en déploiement automatisé. Admettons que tout ce qui est sur master soit instantanément envoyé sur le serveur de production, se rendre compte que les tests unitaires ne passe plus une fois les branches mergées veut potentiellement dire que l'on a pété l'application de production. Dommage.

Bref, si les tests unitaires sont exécutés automatiquement pour chaque PR, il est simple d'en vérifier les résultats avant de merger la PR.

Soyez bienveillant

J'ai lu (il y a plusieurs années déjà) un article de Gilles Roustan intitulé « La revue de code bienveillante ». J'adhère à peu près à tout ce qu'il dit dans cet article, donc je vais surement un peu paraphraser, mais je vous invite quand même à lire son article pour approfondir le sujet.

En gros, soyez gentil. Lorsque l'on fait une revue de pull request tout se passe par écrit et il manque donc la plus grosse partie de la conversation : le langage non verbal, corporel. Difficile de savoir sur quel ton est donnée une revue lorsque l'on n'entend pas la voix de celui qui la donne. Le problème, c'est que notre cerveau va nous faire lire les messages avec un ton qui dépendra de ce qu'il peut capter comme informations : notre environnement, ce que l'on vient de vivre, la musique que l'on écoute, ce que l'on est en train de manger. Vous venez de vous faire remonter les bretelles par votre patron ? La revue aura peut-être un air de reproche. Mais une personne d'autre n'aura surement pas la même impression en lisant le même texte.

Tout cela pour dire que le lecteur doit sentir la bienveillance dans votre revue de code. Eviter l'impératif lorsque vous énoncez une idée d'amélioration, mais proposez des solutions. Préférez les pronoms « nous », « nos » aux pronoms « tu », « ton ». Dans le doute, utiliser des articles définis ou indéfini. « Le code n'est pas très propre » fait moins accusateur que « ton code n'est pas très propre ». Précisez qu'il s'agit de votre opinion, pas d'une science absolue et que vous êtes ouvert à discussion. Si ce n'est pas le cas, expliquez clairement pourquoi.

Ce n'est pas grand-chose, mais ce sont de petits détails qui permettent de ménager l'égo de chacun et donc que la pull request atteigne ses buts : éviter les régressions, freiner les erreurs de conception, faire progresser chaque membre de l'équipe, partager les connaissances.

Faites les plus petites pull requests possibles

S'il y a une chose à laquelle je tiens tout particulièrement lorsque l'on parle du travail collaboratif avec Git, c'est celle-ci :

Faites les plus petits commits et les plus petites pull requests

Pourquoi j'y tiens ? Tout simplement parce que ça sauve la vie des mainteneurs du projet ainsi que celle de vos reviewers.

Limitez-vous à une fonctionnalité par PR. C'est vraiment important. Lorsque je dois faire la revue d'une PR qui contient des modifications sur 266 fichiers, je sais que je vais prendre trente ans à la lire, je sais qu'il y à sûrement plusieurs fonctionnalités et que je vais donc avoir du mal à suivre le raisonnement de l'auteur. Je sais que je n'ai pas envie de faire cette revue de code.

C'est comme si un auteur décidait d'écrire un livre qui contient 3 histoires complètement différentes qui se chevauchent à un rythme non-régulier. Parfois sur une même page, on peut retrouver une portion de chaque histoire. C'est simple, je ne lis pas le livre.

Pensez aux autres développeurs du projet. Ils n'ont pas que ça à faire. Eux aussi veulent coder et si vous leur donnez des pull requests qu'ils mettront une journée à comprendre, ils risquent de ne pas vous apprécier longtemps. Faites des pull requests courtes.

Il vaut mieux faire 15 PR de 3 lignes qui concernent chacune un sujet / une fonctionnalité bien définie qu'une seule PR qui contient les 15 modifications. Elles recevront souvent plus d'attention de la part des autres développeurs et seront donc vraiment utiles.

Et juste pour dire un mot sur la taille des commits. Utiliser le même principe pour commiter : plus c'est petit, mieux c'est. Mon conseil est de penser à ce que vous allez écrire dans la description du commit. Si vous prévoyez d'écrire un « et » ou un « + » ou même une liste à puce, c'est que votre commit est trop gros. Découpez-le. Vous pourrez toujours nettoyer votre historique et fusionner certains commits entre eux avant de faire votre pull request.