L’objectif principal de la code review est de repérer le code difficile à maintenir

• La code review ressemble moins à une procédure destinée à traquer les bugs ou à garantir l’intégrité qu’à un processus visant à faire ressortir à l’avance le code qui sera difficile à maintenir plus tard
• Si même le reviewer a du mal à lire et comprendre le code, il y a de fortes chances qu’un futur mainteneur rencontre le même problème
• Il vaut mieux corriger le code maintenant, tant que son auteur d’origine se souvient encore du contexte, et il n’est pas réaliste d’espérer trouver les bugs de manière fiable par la seule inspection du code
• Pour un reviewer, la consigne « essaie de comprendre, puis signale ce que tu ne comprends pas » est plus actionnable que « trouve des bugs »
• Une bonne review ne consiste pas à tout prouver parfaitement, mais commence par laisser des notes sur les points peu clairs et demander des améliorations

---

Recentrer la code review

• L’objectif central de la code review n’est pas que le reviewer trouve des bugs, ni de garantir que le code est exempt de bugs
• En pratique, il est peu réaliste d’attendre qu’on puisse généralement détecter les bugs en parcourant simplement le code
• Le cœur de la review se situe donc moins dans « est-ce que ce code est correct ? » que dans « est-ce qu’une autre personne pourra le lire et le modifier plus tard ? »

Le code difficile à comprendre est un signal de risque de maintenance

• Le reviewer lit le code pour essayer de comprendre ce qu’il fait et comment il fonctionne
• Les parties difficiles à comprendre deviennent des signaux de risque indiquant qu’un futur mainteneur pourrait s’y retrouver bloqué
• Il vaut mieux corriger ce type de code immédiatement, tant que l’auteur d’origine a encore le contexte en tête

Une mission de review plus actionnable que la chasse aux bugs

• Demander « cherche des bugs dans ce code » est une tâche dont il est difficile d’évaluer le succès
• Même si l’on trouve quelques bugs, d’autres peuvent rester cachés, ce qui fait qu’aux yeux du reviewer, seul l’échec devient vraiment tangible
• À l’inverse, la consigne « essaie de comprendre ce code, et signale ce que tu ne peux pas comprendre » est plus claire
  • Il n’est pas nécessaire de tout comprendre parfaitement
  • Il suffit de consigner les points incompris
  • Si l’on essaie de comprendre l’ensemble et que l’on laisse des notes là où l’on bloque, on remplit déjà le rôle de la review

Les critères de review dans la pratique

• Un code que le reviewer ne parvient pas à comprendre peut, en soi, devenir un candidat à la modification
• Les commentaires de review ne servent pas seulement à signaler des bugs, mais aussi à faire ressortir un manque d’explications, des problèmes de structure ou un enchaînement difficile à lire
• Selon ce critère, il est plus important de mettre le code dans un état que les autres membres de l’équipe pourront ensuite lire et manipuler, que de tenter d’en prouver la validité

La découverte de bugs est un effet secondaire

• Cela ne signifie pas que la code review ne trouve jamais de bugs
• Des bugs peuvent être découverts pendant la review, mais il est difficile d’en attendre qu’elle trouve tous les bugs, ou même la majorité d’entre eux
• Un critère de réussite plus réaliste consiste à vérifier la compréhensibilité du code et à améliorer immédiatement avec l’auteur d’origine les parties difficiles à maintenir

[GN⁺]

Avis sur Hacker News

• La revue de code n’a pas un seul objectif. Repérer le code difficile à maintenir est important, mais ce n’est pas son seul objectif, et peut-être même pas le plus important.
  C’est un garde-fou qui rend plus difficile la fusion de code malveillant, même si un développeur ou une IA part dans une direction étrange, et une personne qui n’est pas trop proche du problème peut voir une meilleure approche ou un problème passé inaperçu.
  Quelqu’un qui connaît mieux d’autres parties du système peut aussi détecter des problèmes d’interaction, cela permet à au moins une autre personne de se familiariser avec le code, et c’est une occasion d’apprentissage pour l’auteur comme pour les reviewers.
  C’est particulièrement important quand il y a un écart d’expérience : quand je mentore de nouveaux employés, je les ajoute comme reviewers à toutes mes PR pour qu’ils voient ma façon de travailler, et je relis aussi leurs PR pour les orienter. Parfois, moi aussi, j’apprends d’eux.
  Oui, cela permet aussi de trouver des bugs, mais ce ne devrait pas être le mécanisme principal ; c’est particulièrement important pour les bugs de sécurité et de performance, difficiles à détecter avec des tests automatisés.

  • Pour ajouter un ancien argument : les gens écrivent le code différemment quand ils savent que leur code sera relu et que ce code contribuera à former, sur la durée, une impression de leur compétence et de leur adéquation à l’organisation.
  • Comme pour le point 3, une personne plus familière avec ce sous-domaine peut repérer ce qui ne correspond pas au code existant ou ce qui pourrait poser problème.
  • J’ai compris « objectif unique » comme : comprendre le code et signaler les parties que l’on ne comprend pas. Si l’idée est qu’une fois qu’on comprend, on peut pointer ce qui est incorrect, idiot ou dangereux, alors ça me paraît convaincant.
    C’est particulièrement vrai pour la modularisation et la décomposition : une fois qu’on comprend l’ensemble d’une énorme PR, on se construit un modèle mental et l’on commence à voir si ce sera maintenable, si cela deviendra un cauchemar total un jour, ou quelque chose entre les deux.

• À mon avis, l’aspect peut-être le plus important de la revue de code est le transfert de connaissances.
  Dans notre petite équipe, sauf urgence, toute l’équipe approuve une PR avant fusion, ce qui fait que chaque membre a une idée générale de l’état actuel de la codebase. Cela permet d’éviter les mauvaises surprises que j’ai connues dans des entreprises plus cloisonnées, du genre « tout le système dont je dépendais a disparu ».
  C’est aussi un lieu où poser des questions sur le fonctionnement et approfondir sa compréhension. Dans une équipe qui fonctionne bien, tous les développeurs devraient comprendre dans une certaine mesure l’ensemble du système, y compris les parties qu’ils ne modifient pas directement.
  Une autre fonction importante est la vérification des connaissances organisationnelles. J’ai récemment modifié légèrement une table, et un collègue m’a signalé qu’un microservice auquel je n’avais pas pensé écrivait dans cette table et que cela casserait quelque chose. Je ne savais même pas que ce microservice existait ni qu’il accédait à cette table, mais cette vérification a permis d’éviter un problème plus important et une situation de nettoyage de données.

  • « Toute une petite équipe qui approuve les PR avant fusion », ça semble bien pour une petite codebase qui évolue lentement, mais si on l’impose à une grande équipe ou à une codebase qui bouge vite, cela risque vite de devenir un rituel purement formel où l’on survole le code et clique sur approuver.
    À la fin, tout le monde est occupé par son propre travail et ne veut pas être la personne qui bloque une PR importante, donc on se contente d’approuver ; en réalité, plus personne ne relit le code.
  • Je me demande quelle est la taille de l’équipe. Au-delà d’environ cinq ingénieurs, cette approche ne me semble probablement pas extensible.
    Pour des problèmes du type « tout le système dont je dépendais a disparu », je pense que les tests automatisés capables de détecter cela même si la personne qui dépend de ce système n’est pas là sont très importants.
    Je défends aussi fortement la propriété partagée de tout. Il est naturel que les gens finissent par posséder de facto certaines parties de la codebase, surtout les composants qu’ils ont créés, mais cela mène aux silos et à un faible bus factor. Il ne faut pas se retrouver avec une personne qui possède un système, lequel dépend à son tour d’un composant possédé par une seule autre personne.
  • Je me demande ce qui aurait empêché ce changement cassant d’arriver en production si ce collègue n’avait pas été dans la revue. Si la revue de code est importante, où se placent les tests ?
  • Il m’arrive de créer une PR et de taguer d’autres développeurs même pour du code que je vais fusionner immédiatement de toute façon. L’objectif est de fournir un chemin facile pour voir ce qui a été fusionné et pourquoi, afin que les gens ne passent pas à côté de ce qui se trouve dans la codebase.
  • Pour les changements non triviaux ou les travaux de nettoyage, un second regard est toujours utile. Mais l’approche « tout le monde lit tout » ne passe pas à l’échelle quand N devient grand.
    Quand il y a trop de choses à lire, plus personne ne peut suivre ; c’est pour cela qu’on délègue, qu’on documente et qu’on organise des sessions de vue d’ensemble.

• J’ai toujours pensé qu’il valait mieux considérer la revue de code comme le point de passage où le code qui appartenait à son auteur devient la propriété de l’équipe ou du projet.
  Le code que je relis n’est pas ton code, c’est du code qui va bientôt devenir notre code. Évidemment, la maintenabilité en est un élément majeur.

  • C’est vraiment un environnement enviable et luxueux.
    Notre équipe a commencé à utiliser l’IA, alors j’ai adopté une approche simple. Je ne laisse pas de commentaires : je prends seulement une décision d’approbation binaire, « est-ce complètement délirant, ou est-ce que ça peut passer ? ».
    J’économise mon temps et ma santé mentale.

• Cela ne fera que rendre le reviewer et l’auteur plus paresseux
  L’objectif d’une revue de code est multiple. Est-ce difficile à maintenir, peut-il y avoir des bugs, peut-on faire plus simple et plus propre, est-ce conforme au style de code du projet, est-ce que cela permet à d’autres personnes de comprendre le code, est-ce que cela aide à onboarder les juniors de l’équipe, est-ce que cela sanity check les décisions de conception : tout cela en fait partie.
  Ce genre d’article léger ressemble surtout à une façon pour des reviewers de code paresseux de se justifier

  • Il existe une checklist spécifique pour ce qu’il faut regarder en revue
    Il faut vérifier si le changement atteint fonctionnellement l’objectif décrit dans l’issue ou la PR, s’il reste du code inutile comme des sorties de debug ou des clés d’API privées, s’il y a des défauts évidents comme des fuites mémoire, des cas limites non traités, des failles de sécurité ou des appels à des API obsolètes.
    Il faut aussi regarder si l’on peut rendre le code plus compréhensible, s’il faut ajouter ou retirer des abstractions, si les noms de variables et de méthodes sont meilleurs, et s’il serait préférable d’utiliser plus, ou moins, un style fonctionnel.
    Il faut également vérifier la cohérence avec la codebase ou le guide de style, l’existence d’améliorations de performance évidentes, comme utiliser un ensemble de hachage au lieu d’une liste ou appliquer une évaluation paresseuse, ainsi que la suffisance des tests.
    Je ne suis pas non plus entièrement d’accord avec l’idée qu’un code incompréhensible ne devrait pas passer. Certains codes sont tout simplement très difficiles à comprendre, et l’objectif est qu’ils soient fonctionnellement corrects tout en étant aussi compréhensibles que possible.
  • Le secteur a beaucoup travaillé pour passer de « blâmer l’auteur » à « blâmer le processus et l’équipe »
    Mais cet article ressemble surtout à un appât, un peu comme dire : « les gens pensent que le dîner consiste à manger de la nourriture, alors qu’en réalité il ne s’agit pas de manger, mais de se connecter avec sa famille et ses amis ». C’est un certain type de raisonnement réductionniste bancal qui marche bien sur HN.
  • Maintenant qu’avec l’IA la vitesse à laquelle on écrit et déploie du code s’est accélérée, l’accent devrait se déplacer vers la revue. Il faut vérifier que le code s’exécute réellement correctement, que toutes nos hypothèses sont justes et qu’il n’y a pas d’effets de bord involontaires.
    J’ai eu l’impression que la revue et le debug prenaient beaucoup plus de temps que l’écriture et la production de code, et se contenter de « prier pour que ça marche » ne finit jamais bien.

• Dire qu’« on ne peut généralement pas trouver de bugs en examinant le code » est complètement faux. On peut en trouver largement à chaque niveau d’abstraction, et c’est ce qu’on appelle des code smells
  Des descripteurs de fichiers non fermés, des coroutines non awaitées, de gros blocs try/catch qui retournent une certaine valeur sans journaliser l’erreur, de mauvais casts de types, etc., en font tous partie.
  En règle générale, il ne faut pas voir le type checker, le compilateur et le runtime comme de simples étapes à franchir. Il faut travailler avec eux, reconnaître que ce sont des outils précieux, et ne pas travailler à rebours contre eux.

  • Je ne vois pas de quoi il parle. Il m’est déjà arrivé de repérer des bugs uniquement par revue de code, sans exécuter le programme ; d’autres l’ont fait pour mon code ; et j’ai aussi vu cela dans les revues d’autres personnes.
    On peut sans doute définir « généralement » d’une manière techniquement vraie, mais cela rend alors l’affirmation peu significative.

• Si l’on veut formuler des affirmations générales sur la revue de code, il est important de définir de quel type de revue de code on parle
  Aujourd’hui, la revue asynchrone de PR à la GitHub avec commentaires inline est la norme, mais la revue ne se résume pas à cela. Autrefois, il existait aussi des revues en face à face, avec des processus proches d’une soutenance de thèse ou d’une présentation en conférence.
  La littérature qui présente la revue de code comme une pratique qualité utile — en fait comme l’une des rares pratiques qualité utiles — provient généralement de processus de revue beaucoup plus structurés que ceux d’aujourd’hui.
  Personnellement, avant les LLM, les revues de PR à la GitHub me semblaient surtout servir à donner l’impression que le processus était correct ou à cocher une case de gouvernance ; à l’ère des LLM, leur rapport coût-efficacité va devenir bien pire, et je pense qu’elles disparaîtront.

  • Les revues synchrones sont toujours possibles. L’un de mes premiers managers m’a appris que si une revue de code « standard » faisait plus d’un aller-retour, il valait presque toujours mieux se voir en personne, ou passer par Zoom si au moins une personne était à distance, puis consigner l’accord trouvé dans un commentaire.
    Pour prendre une analogie technique un peu forcée, la communication textuelle asynchrone est plus lossy que la parole quant à l’information qu’elle permet d’encoder avec succès, et son débit est plus faible. Donc lorsqu’il faut échanger beaucoup d’informations, cela vaut la peine de payer le coût de synchronisation.
  • Dans l’un de mes premiers emplois, il fallait imprimer les packages de changements pour les relire et les signer. Il y avait aussi quelqu’un chargé de conserver la version finale dans un classeur.
    C’était plus proche de l’ingénierie traditionnelle, et tout le monde devait considérer le logiciel comme quelque chose de plus permanent.

• Garantir la maintenabilité est bien l’un des bénéfices de la revue de code, mais dire que c’est son seul objectif est une affirmation assez audacieuse
  La revue de code est aussi un outil qui permet à l’équipe de comprendre les changements de code et de partager une responsabilité collective sur l’ensemble de la codebase.

• La revue de code n’est pas une chose unique. Il y a de nombreuses raisons d’en faire : partage de connaissances, dilution de responsabilité, qualité du code, conformité réglementaire, etc. Et comme toujours, l’objectif dépend du contexte d’utilisation

  • Dire que la plupart des gens ne comprennent pas la raison des revues par les pairs me paraît assez ignorant. Le fait de croire qu’il n’y a qu’un seul objectif ressemble en soi à un signe de manque d’expérience avec d’autres équipes ou d’autres personnes.

• Si l’auteur est mathématicien, l’affirmation « on ne peut généralement pas trouver de bugs en examinant le code » ne veut probablement pas dire qu’il est totalement impossible de trouver des bugs
  Cela signifie plutôt qu’il n’est pas possible de trouver tous les bugs, ou de trouver un bug donné.

  • D’après mon expérience des cours de maths à l’université, les mathématiciens sont souvent vraiment mauvais pour communiquer avec les autres humains. Cela explique pourquoi ce qu’il pense avoir dit peut différer de la manière dont presque tout le monde le lit.
  • Dans ce sens, c’est vrai
    Pour le relier au point sur la maintenabilité, l’un des objectifs de la revue est aussi de rendre le code aussi simple que possible afin d’augmenter la probabilité qu’il soit débogable par revue. Cela n’empêche pas les bugs au sens absolu, mais cela augmente les chances.
  • L’auteur mathématicien semble ne pas comprendre le sens de ses propres quantificateurs en langage naturel. « On ne peut généralement pas trouver de bugs en examinant le code » signifie « en examinant le code, on ne peut généralement trouver aucun bug », et non « on ne peut pas trouver tous les bugs ».
    La première interprétation est pertinente mais fausse, la seconde est vraie mais non pertinente.
    Il voulait probablement dire : « pour un bug donné, on ne peut généralement pas le trouver en examinant le code », c’est-à-dire « pour tout bug B, il n’est pas vrai que l’on puisse trouver B » ; mais cela aussi est vrai tout en passant à côté du sujet.

• J’ai travaillé à la fois avec des personnes qui refusaient souvent les suggestions sur les PR, et avec d’autres qui les acceptaient.
  Quand quelqu’un accepte les suggestions, je me demande dans quelle mesure cette personne cherche à partager la propriété du code avec moi. On a l’impression que nous maintenons et comprenons tous deux le code, et que nous regardons dans la même direction.
  À l’inverse, face aux PR de quelqu’un qui rejette les suggestions, l’envie de participer diminue. Si elles seront refusées de toute façon, pourquoi prendre le temps de faire une revue minutieuse ?

  • Dans notre équipe, nous ajoutons généralement devant les commentaires des préfixes comme thought, change, nit, fix, chat.
    Par exemple, thought signifie quelque chose comme « foo pourrait devenir plus courant plus tard, donc refactorisons à ce moment-là » ; change, « c’est une abstraction qui fuit, j’aimerais qu’on la modélise comme bar » ; nit, « le nom n’est pas très intuitif, envisageons Baz ou Boo » ; fix, « ce test unitaire vérifie le mauvais champ » ; chat, « c’est une décision importante qui déterminera la forme des solutions de cette catégorie à l’avenir, discutons-en d’abord avec l’équipe ».
    Certains préfixes bloquent la PR tant que ce n’est pas corrigé ; d’autres indiquent que le commentaire peut être pris en compte ou non. Cela clarifie pour l’auteur ce qui relève de « parvenir à la même compréhension » et ce qui relève d’une « préférence d’expression » ou d’une « observation ».
    En revanche, si vous laissez un nit et que l’autre personne n’est pas d’accord et l’ignore, il ne faut pas le prendre mal. Si vous y teniez fortement, ce n’aurait pas dû être un nit.
  • Si vous estimez qu’un point est important, il faut laisser une suggestion bloquante et forcer la discussion.