Ce que j’ai appris de mes 3000 Code Reviews

Les choses à faire, à ne pas faire et comment progresser.

Spéciale dédicace à ceux qui voulaient un article sur mes vacances en Italie! TLDR; c’était cool 😆

Je réfléchis à écrire cet article depuis quelques semaines déjà, d’abord pour aider mes collègues dans leur travail de relecture, mais aussi parce que j’ai le sentiment que pour beaucoup de développeurs cette tâche est soit mal comprise, soit perçue comme ingrate.

Mais qu’est-ce que la Code Review ou autrement dit la relecture de code?

Pour beaucoup, peut être une très grosse majorité de développeurs, voici une définition tout à fait valide:

Tâche qui consiste à analyser le code pour détecter les erreurs et les bugs, que ce soit par rapport à des problèmes fonctionnels (le code ne fait pas ce qu’il devrait faire), des régressions (le code a cassé un comportement existant), une erreur syntaxique (un chevron oublié, une variable non définie appelée) ou encore le respect des standards du projet (l’indentation, les normes de nommages, les standards propres au langage) etc etc…

C’est donc une situation où la personne qui relit juge le travail d’un/une collègue de travail, voir d’une personne extérieure dans le cadre d’un projet open source et doit pointer tout ce qu’il y a de négatif dedans.

Evidemment, si c’est ce type de relecture de code que vous attendez de vos collègues, vous aurez de grosses difficultés à trouver un/une volontaire.

Sans remettre en question cette définition, je la trouve limitée et parfois un peu fausse: je vais aborder avec vous d’autres utilités de la relecture de code.

La Code Review est une source de connaissance

La majorité des relectures de code que je demande, ou que j’effectue sont réalisées dans ce cadre. Par exemple, sur le projet PrestaShop il n’y a pas une seule contribution acceptée qui n’échappe à l’oeil de Sauro… euh de Mickaël Andrieu. Selon GitHub, cela représente 40% de mes contributions et selon moi 60–70% de mon temps de travail actuellement.

Pourquoi? Parce qu’on le sait tous que tout ne sera pas documenté, que tout ne sera pas testé et pourtant il faudra bien quelqu’un/quelqu’une soit capable de maintenir tout ce qui aura été accepté sur la branche de développement :)

Et sincèrement, c’est une illusion de penser que la personne qui a réalisé la contribution sera disponible le jour où on en aura besoin, en tout cas très naïf dans un monde où la plupart des développeurs changent de boîte tous les 2 ans.

Et c’est lors de cette code review que je peux poser des questions sur le contexte, sur le “pourquoi” on fait ça et si on a fait un truc un peu sale, d’en expliquer les raisons. Ce n’est pas rare de me voir écrire un “For the record …” (pour l’historique) parce que le jour où il faudra remonter jusqu’à la contribution pour comprendre POURQUOI “ce con” (merde, c’était moi xD) a codé ça comme ça, c’est utile d’avoir le contexte de l’époque…

Mais pourquoi il a touché ce template? Ah… ok!

La Code Review est un moyen de communication

Une chose que j’ai de plus en plus intégré dans mes Code Reviews, c’est l’avis de personnes “non développeur” en prenant un bout de code et en expliquant ces tenants et aboutissants pour avoir leur avis sur le sujet.
Par exemple, lors de l’ajout d’une nouvelle fonctionnalité, je me rends compte que l’implémentation choisie aura une conséquence sur une autre fonctionnalité. A ce moment là, je “ping” quelqu’un de l’équipe Produit, en lui expliquant les avantages et inconvénients de la solution choisie et lui demandant de trancher.

Idéalement, dans les Code Reviews j’aimerai que toutes les personnes impliquées interviennent: l’architecte pour valider que l’implémentation ne casse pas les choix d’architecture choisis, un content manager pour valider les contenus, un développeur pour repasser sur des erreurs de code ou des optimisations, un manager “Produit/QA” pour confirmer que c’est ce qui est attendu… il n’y a pas de raison, au vu des outils que nous avons maintenant d’exclure certaines personnes des processus de relecture de code.

C’est d’ailleurs ce qu’il se passe déjà chez PrestaShop où selon les contributions il y a:

  • une personne chargée de valider les clés de traductions;
  • parfois des membres de la communauté qui donnent leur avis;
  • au minimum un, voire 2 développeurs selon la taille de la contribution;
  • une personne de l’équipe Produit parce qu’il faut que le logiciel garde une certaine cohérence;
  • parfois “moi” qui contrôle qu’on casse pas l’architecture que j’essaie de mettre en place;

… et pour conclure l’équipe Qualité & Assurance sans qui rien n’est accepté de toute façon!

Et de sept participants au courant de cette fonctionnalité!

Beaucoup de monde communiquent autour d’un bout de code, et c’est très bien 👼

La Code Review pour améliorer la qualité du Code?

Evidemment que c’est toujours un des objectifs de la relecture de code, mais pour ma part j’ai tendance à en limiter le scope (ou à me limiter). Soyons clairs, on peut TOUJOURS améliorer le code, la question est: quand est-ce qu’on estime que c’est suffisamment bon?

Sur ces questions là, dès qu’un code est suffisamment extensible, couvert par les tests et qu’il ne présente pas de risques de bugs évidents: pour moi c’est bon.

Ce qui m’importe plus, c’est le nommage, la structure et la documentation: il est important d’avoir une constance, surtout pour les très gros et vieux projets dans lesquels il est compliqué d’entrer pour comprendre comment ça fonctionne.

Parfois, un bon nommage, ou un ré-usinage d’un bout de code en une ou plusieurs fonctions bien nommées (et avec une seule responsabilité) peuvent limiter le besoin d’écrire de la documentation.

J’ai une règle très personnelle à ce sujet: ce qui n’est pas documenté n’existe pas.

J’ai aussi décidé de bannir de PrestaShop tout ajout de fonctions avec un argument de type booléen: non seulement dans 3 semaines plus personne ne saura ce que change cet argument, mais surtout si le comportement de la fonction change en fonction d’un booléen c’est … qu’elle a deux comportements 😃

Plus j’avance dans mes code reviews, plus je souhaite des fonctions courtes idéalement avec 0 ou 1 argument et des classes courtes avec 1 seule responsabilité: c’est con mais le problème des gros projets legacy, ce n’est pas d’avoir des milliers de fichiers mais c’est de comprendre et maîtriser rapidement quel est le rôle de chaque fichier…

Et les standards de code dans tout ça?

C’est pas compliqué, c’est une immense perte de temps! Faites comme Symfony, comme Api Platform, comme PrestaShop ou tout autre projet sérieux: mettez en place l’outil de linter/fixer qu’il vous faut, essayez de pas inventer vos propres standards et laissez un bot s’occuper de ça!

On en a ENFIN terminé avec ce type de Code Review chez PrestaShop \o/

Car s’il y a bien un truc qui m’énerve et qui devrait tous vous énerver, c’est de voir un bon développeur perdre une heure et demi sur une contribution et remonter uniquement des espaces, des virgules et des accolades non alignées.

Comment progresser en “code review”?

Voici une liste de petites choses que j’ai effectuées/comprises depuis quelques mois et qui ont eu un impact positif:

  • Pointer les erreurs, et aussi les choses que vous trouvez bien faites (ex: wow, nice use of ***);
  • Dans tous les cas, remerciez les gens qui relisent et valident votre code;
  • Dans tous les cas, remerciez la personne qui fait la contribution, même si vous la refusez (plus propre à l’open source, m’enfin…);
  • Contextualiser votre contribution! On dit RTFM, et pas RTFC pour une bonne raison: le code ne contient pas toute la connaissance métier et ce même si vous êtes un DDD addict (mais ça aide 👊);
  • Ne dites jamais un truc du genre “it’s stupid” et si vous devez émettre une critique forte dites “this code/this block” pour bien pointer le travail et non la personne;
  • J’insiste encore une fois sur la bienveillance, l’humour peut aider si vous êtes un connard comme moi;
  • Si quelque chose n’est pas clair, n’hésitez pas à rapatrier le code sur votre machine et à essayer: ça m’arrive de plus en plus de demander à mes collègues “t’as vérifié que ça marche ou t’as juste lu le code?” et souvent, souvent… certaines relectures de code nécessitent de vérifier le comportement induit. C’est pas du bonus, ça fait parti du travail attendu 😜;
  • Faire une code review a plus de valeur que d’écrire du code. Pourquoi? Parce qu’une contribution, tant qu’elle n’est pas acceptée n’a aucune valeur: il faut mieux 20 contributions réalisées dont 10 acceptées par semaine, que 70 contributions réalisées et 5 acceptées. Cette règle, quel que soit le projet est toujours vraie;

Et vous, comment organisez-vous vos relectures de code? Il me tarde d’écouter vos différents points de vue sur le sujet!