Comment bien donner et recevoir une code review (sans drama)
La code review est l’un des outils les plus bénéfiques et formateurs pour un développeur. Sauf quand c’est mal fait. Là, ça devient vite le drame. Il y a des choses à savoir humainement et techniquement pour réussir une code review. Que ça soit pour la donner ou la recevoir.
Litige
Il y a bien longtemps, dans une galaxie lointaine, très lointaine je bossais dans une équipe d’une dizaine de développeurs. Lorsque du nouveau code devait être intégré dans le système, il passait d’abord par la grande instance de la code review.
La juridiction compétente pour nous autres développeurs.
Et ce jour-là, c’est exactement ce qui se passe. Un de mes collègues envoie sa code review à un autre. Les discussions restent entre ces deux personnes. En apparence, tout se passait comme les textes de loi l’avaient prévu. En vérité, cette review n’était pas comme les autres.
Dans la confidentialité de cette pull request, le plus grand drame de l’équipe venait de commencer.
Au cours des jours suivants, j’ai senti que l’ambiance se détériorait. Progressivement, les visages se sont fermés. La communication a pris du sursis.
Un beau matin, le collègue qui avait reçu la code review se tourne alors vers moi.
« – Je t’ai assigné la code review de Mr. X. J’en peux plus, ça me soule. Y’a rien qui va et il fait pas ce qu’on lui demande. Ça fait plusieurs jours, je veux plus avoir à gérer ce bordel. »
J’ai pas vraiment eu de réaction. Je ne comprenais pas encore ce qui se passait. En ouvrant cette fameuse PR sur GitLab, j’ai vite compris.
Que ça soit d’un côté ou d’un autre, c’était exactement ce que tu ne veut pas faire ou dire lors d’une core review.
Énormément de code. Aucun contexte. Pas de tests. Des commentaires violents. Des débats sans fin. Ça durait depuis une semaine et ça grossissait à vue d’œil. Y’avait le feu partout et les dégâts étaient déjà graves.
Cette code review à elle toute seule sera le point de départ du drame qui transformera l’équipe.
Cour d’assises
Si j’utilise autant le champ lexical de la justice dans l’audience que tu m’accordes aujourd’hui, c’est pour une bonne raison. Tout le monde voit la code review comme un tribunal. Une affaire, un accusé et des jurés.
Une bonne fois pour toutes, il faut arrêter avec cet état d’esprit. La code review ne sert pas à juger. La code review sert à l’amélioration de la qualité de l’application et aux partages des connaissances dans l’équipe. Plus concrètement, on veut trois choses.
Repérer les bugs
L’erreur est humaine. Il faut plusieurs cerveaux pour maximiser les chances d’éviter les problèmes. Même avec plusieurs cerveaux, les bugs sont toujours là. Et ça, c’est bien la preuve que la code review est absolument nécessaire.
Propager la connaissance
J’ai rien contre un certain ownership de bout de système par certaines personnes. Mais ce qu’on veut c’est que tout le monde puisse intervenir un minimum partout. Et la code review, c’est juste parfait, taillé sur mesure pour permettre ça.
Amélioration de la qualité
Dans mon article sur les bonnes équipes de développeurs, je disais que la qualité est un effort d’équipe. Une discipline de groupe, une tradition forte qui devrait être sauvagement défendue par tout le monde. Cet effort passe énormément par la code review.
La code review est censé être un échange de savoir bidirectionnel, une discussion, pas un tribunal correctionnel.
Faites entrer l’accusé
Quand Mr X à vu que la code review m’a été assigné, il a vu rouge.
Il y a une énorme différence entre une code review privée entre deux personnes, et une code review qui devient publique. En particulier quand ça se passe mal.
Et la situation s’est brutalement aggravée quand Mr X nous a montré du doigt auprès de la direction. Je dis nous, car même si je n’avais même pas encore participé, je faisais déjà partie du camp adverse.
En nous montrant du doigt, il traça une ligne dans le sable. À partir de ce moment-là, plus personne ne franchit cette ligne.
C’était trop tard, il n’était plus question de code.
Plaidoirie
Pour éviter ce genre de situation infernale, voilà quelques règles indispensables pour celui qui envoie la code review.
Envoyer du code fonctionnel et testé
Ça semble évident. Ça ne l’est pas. Le problème principal étant la pression mise sur les développeurs au niveau du temps. En sautant quelques principes de base, c’est sûr que ça va plus vite.
Une manière efficace d’y arriver et de toujours se mettre à la place de la personne qui va regarder ta code review. Faire la code review de ton propre code comme si c’était le code de quelqu’un d’autre.
Faire ça -juste ça- propulse ta code review dans le top 1% monde.
Envoyer le moins de code possible
Le moins de code tu enverras, le mieux ça sera. La personne qui va la recevoir va être reconnaissante et du coup efficace. Ça permet également de mieux contrôler les changements apporté à ton application et de limiter les bugs.
Une manière efficace d’y arriver et de travailler en amont pour réduire le scope des tâches.
Si tu vois qu’une tâche va nécessiter énormément de code, insiste pour qu’elle soit découpée en plusieurs tâches.
Envoyer d’abord le contexte
Il n’y a rien de pire que d’envoyer du code à quelqu’un sans aucun contexte. Tu imposes un double travail à ton collègue. Si tu veux des efforts de la personne, tu dois faire des efforts de clarification en amont.
Une manière efficace d’y arriver et de mettre une bonne description du problème que règle ta solution dans la description de ta code review. Pourquoi pas un lien direct vers le ticket, si le ticket est bien renseigné.
Tout ce dont la personne aura besoin pour optimiser son temps de compréhension est précieux.
Prendre du recul
Une code review a pour objectif de challenger ta solution. On va challenger ton code, pas toi personnellement. Les gens qui n’ont pas ce recul sont incapables de faire une code review normalement.
Une manière efficace d’y arriver et de te souvenir que tu n’es pas ton code. Que rien n’est parfait et que chaque critique est une opportunité d’amélioration. Pas une attaque personnelle.
L’incroyable égo des développeurs est fortement mis à l’épreuve ici.
Jurés
Il y a également des règles indispensables à respecter pour celui qui reçoit la code review.
Exiger le minimum
Pour faire ton travail de revue efficacement tu dois mettre certaines barrières à l’entrée. Ces barrières devraient être les mêmes pour tout le monde. Tu veux optimiser l’utilisation de ton temps et de celui qui t’envoie la review.
Si tu ne vois aucun test, aucun contexte, tu la renvoies poliment. Avant de faire la revue, tu checkout la branche et tu lances les tests chez toi. Une fois que tu es sur du sérieux de la code review, tu y consacres du temps.
Poser des questions
Beaucoup de développeurs oublient que les commentaires qu’ils laissent sont lus par un être humain.
La façon dont tu fais tes retours et aussi importante que tes retours eux-mêmes. Si tu es un connard, mais que tu as raison, tu es un connard qui a raison. Et devine quoi ? Personne ne veut travailler avec un connard.
J’ai une manière très efficace pour avoir du tact dans le cadre d’une code review. Il suffit de poser des questions au lieu de donner des ordres.
Concrètement, au lieu de dire « c’est de la merde ta façon de faire ici » tu peux dire « que pense-tu de cette autre manière de faire à la place ? ».
Non seulement tu n’agresses pas ton collègue, mais en plus tu apportes une solution au lieu d’être seulement dans la critique. Crois mois, c’est la botte secrète qui change absolument tout.
En ressenti, tu passes du connard arrogant, à la personne qu’on respecte pour son tact et ces compétences techniques.
En plus, si tu as tort dans ton analyse et que ta remarque était mauvaise : c’était juste une question. Tu pourras dire que tu étais juste curieux et que tu comprends mieux désormais.
Également, celui qui reçoit la code review doit maitriser le côté technique.
Et pour cette partie, il faut commencer par la jurisprudence.
Jurisprudence
Chaque équipe devrait avoir une suite de règle qui régissent comme le code doit être structuré et stylisé dans l’application. Cette jurisprudence est la source de vérité absolue que tout le monde doit respecter. Le rôle de la code review et aussi de faire respecter cette jurisprudence.
Pour te donner un exemple concret, voici la jurisprudence pour tout développeur qui fait du Javascript à Airbnb : https://github.com/airbnb/javascript.
Le même genre de règles pour établir des règles de styles dans ton équipe doit exister.
Si ce n’est pas le cas chez toi, tu devrais le proposer.
Ça mettra fin à tous les débats et les discussions inutiles sur les détails et le style du code. La jurisprudence faisant foi, on peut maintenant s’intéresser au cœur du problème.
Techniquement
Maintenant, techniquement comment tu fais ? Comment tu trouves quoi dire dans une code review ? Comment tu repères du code douteux et comment tu sais quoi proposer comme solution ?
Je ne vais pas te mentir, c’est beaucoup ça aussi qui fait de toi de quelqu’un d’efficace dans le contexte de la code review.
Il y a deux façons d’arriver à cet objectif.
La première est de t’exposer un maximum à ces codes reviews et aux bonnes pratiques de manière générale. Pendant dans années. Apprendre de développeurs plus expérimentés que toi. T’investir dans les code review. En donner, en recevoir ou juste les observer. Tout faire pour participer fortement à ce sujet. A force, tu finiras pas avoir une connaissance et des réflexes affutés.
Mais cette façon là prend du temps. Comment faire pour aller plus vite ?
La seconde manière est un raccourci de la première. C’est ma recommandation du jour : Refactoring : Comment améliorer le code existant.
Ce bouquin est une véritable mine d’or qui m’a appris tout ce que je devais savoir sur l’amélioration du code. Car oui, c’est surtout ça une code review. Une discussion pour améliorer le code qui peut être amélioré.
Les premiers chapitres t’expliquent un peu le principe et les grandes règles du refactoring. C’est intéressant.
Mais le plus intéressant commence au chapitre 3. Ce chapitre est dédié à l’identification formelle du code douteux. Le Saint-Graal, nectar sacré, la source d’efficacité de tes codes reviews.
J’ai dévoré cette partie, je l’ai lut, relue et relue plusieurs fois pour être sûr de bien tout comprendre. J’avais enfin trouvé quoi dire dans les codes reviews que je recevais. Et c’est bien de savoir quoi dire, mais il faut aussi proposer des solutions.
Ça tombe bien, car le dernier chapitre est un catalogue de solution que tu peux apporter à chaque bout de code douteux. Des centaines de pages organisées dans un index pour retrouver facilement ce que tu cherches. J’ai toujours ce bouquin à portée de main.
Cerise sur le gâteau : tout le code pour les exemples dans ce bouquin c’est du Javascript. Aucun prérequis et ca s’adresse aux juniors comme aux seniors.
J’ai personnellement lu la version anglaise, mais tu as une version française disponible aussi !
Dommage et intérêt
Très vite la code review de Mr X avait fait le tour de l’open space.
Les managers ont commencé à poser des questions. Tout le monde se montrait du doigt. La tâche restait dans l’impasse.
Et je dis tout le monde, car je me suis rendu compte que mon collègue qui avait abandonné la review, avait lui-même été assigné dessus par un autre collègue, qui avait abandonné avant lui.
Bien sûr la PR étant désormais publique, tout le monde donnait son avis sur la situation.
Le point culminant étant le jour de la confrontation en personne en plein milieu de l’open space.
L’histoire finie plutôt mal puisque Mr X demanda un transfert dans une autre équipe peu de temps après. Ce fut rapidement accepté et effectif. Il n’était plus question de réparer les dégâts, mais de les limiter.
Pourtant, pour empêcher tout ça, il aurait suffi de faire une seule chose au bon moment.
Quand un drame commence dans une pull request, il faut immédiatement la fermer, et discuter en vrai avec les gens.
Le contexte déshumanisé de la code review exacerbe les réactions, les prises de position et les interprétations. La recette du chaos. Grand destructeur d’équipe.
Que tu le veuilles ou non, une code review est fortement axée sur l’humain et l’ego. Ceux qui ne pensent qu’à la partie technique sont condamnés à produire des drames. Et la situation s’aggrave de façon exponentielle lorsque les deux parties ont cet état d’esprit.
C’est exactement ce qui s’était passer à cette époque.
Cette histoire n’est pas très originale. Elle est déjà arrivée dans beaucoup d’équipes. Et sans un peu de vigilance, ça arrivera dans la tienne.
Épilogue
Une code review n’est pas un tribunal. C’est un échange de connaissances permettant une meilleure qualité et stabilité de ton application. Et évidemment, ça reste une revue technique, mais enlever et/ou piétiner le facteur humain produira un résultat chaotique.
Salut, super article qui décrit bien les soucis d’interprétation qui ont tendance à pop pendant cette phase particulière du développement.
Je mettrais une réserve cependant sur le terme « jurispudence » et l’idée proposée avec. Cela peut vraiment avoir un effet inverse, avec des réactions du type « ben oui on a toujours fait ça » qui sont difficiles à gérer surtout quand on veux amener de meilleures habitudes de qualité via les code review.
Même si il n’est pas question de remettre en cause la suite de règles ou charte de codage définie (elle n’existe pas forcément ou pas toujours expliquée) ça se passe généralement mieux quand celui qui reçoit la revue de code peut faire preuve de pédagogie et être l’avocat de ces règles au delà de commentaires lapidaires comme « pas la bonne manière de faire ».
Très bien le fait de proposer en premier le pdv des devs junior – qui « subissent » la code review! En général, c’est les seniors qui écrivent des blog posts ou parlent aux confs, et ils ont forcement tendance a exposer surtout le pdv des « juges » et analyser tout ce qui y relate…
J’en pense quoi ? Tout d’abord, un grand merci à toi pour ce très bon partage d’xp via cet article ✌
J’attends avec impatience de ne plus être en auto-soumission de code review via mes propres sides-project au profit d’une vraie team qui me permettra de m’améliorer réellement ;
Malgré mon parcours de dév’ junior qui a un autocollant (A) à l’arrière de son véhicule appelé – expériences -, j’avais déjà le même avis que toi concernant cette occasion de partage de connaissances ;
Mais, j’y pense ! D’une certaine manière, j’ai quand même pu soumettre mon code via des retours de CTO lors d’entretiens et, punaise, du pur pain béni pour revoir ma pratique.
C’est pourquoi, à toutes les développeuses débutantes et tous les développeurs débutants (fraîchement diplômés || issus de reconversion … comme moi ^^) || autodictates || etc.) && en quête d’une collab’ …, si vous êtes en galère de retours concernant votre code, bataillez gentiment pour obtenir des retours
Pour moi le role de la code review n’est pas de discuter du style de codage. Si c’est quelques chose d’important dans l’équipe , il faut l’automatiser en pré commit et on en parle plus. Sinon ca donne une charge de travail bien plus importante au relecteur , des remarques pas toujours bien acceptée pour celui est relu pour au final un intérêt discutable.
Je serais curieux de savoir ce que ca représente en temps une équipe qui fait de la code review quelque soit le niveau des dev. Chez nous c’est que les junior/stagiaire qui passe par les review et c’est déjà bien assez long à faire.
Chez nous la code review est obligatoire pour tout le code.
En général, dès qu’une code review est un peu complexe, je privilégie le « Pair Review », ça fait gagné un temps dingue en terme d’aller-retour. C’est à dire que dès qu’il y a beaucoup de code, je ne perd pas mon temps à essayer de comprendre le contexte, je prends rendez vous avec le développeur et par écran partager, je lui demande de m’expliquer l’objectif de son code et je lui pose des questions en live sur ce que je ne comprends pas. Comme tu le dis dans ton article, poser des questions est bien plus accepté humainement et le double résultat « partage de connaissance » et « montée en compétence » des deux personnes est généralement atteint.
Et cet article parfait amène déjà le suivant: La culture du feedback.
C’est pas du code mais si t’apprend la communication non violente alors ta code review sera toujours mieux acceptée.
Salut,
Je traduirais review par relecture, non ? On parle facilement de « relire sa copie ». Ça me semble cohérent. Revue me semble un peu martial comme mot.
Merci pour l’article.