61 points par xguru 2024-08-26 | 13 commentaires | Partager sur WhatsApp
  • La revue de code semble être une bonne idée, n'est-ce pas ?
    • Avec la revue de code, deux développeurs peuvent examiner le code, repérer des problèmes et partager leur compréhension de l'évolution du projet
    • Le relecteur peut apprendre des astuces utiles en examinant en détail le code de l'auteur, ou trouver l'occasion de transmettre des astuces utiles à l'auteur
  • Mais c'est ainsi que procèdent les relecteurs de code du « côté lumineux ». Ils utilisent la revue de code pour améliorer le code et faire progresser collectivement les compétences des développeurs
  • La revue de code peut aussi devenir un outil puissant à des fins totalement différentes. Si le relecteur bascule du « côté obscur », il peut utiliser toutes sortes de méthodes pour empêcher ou retarder l'amélioration du code
    • Il peut aussi poursuivre d'autres objectifs personnels, comme harceler l'auteur du patch ou le décourager complètement
  • Si vous êtes récemment passé du « côté obscur », vous n'avez peut-être pas encore envisagé toutes les possibilités
    • C'est pourquoi cet article propose une liste d'antipatterns pour les relecteurs de code du côté obscur

[Antipatterns]

The Death of a Thousand Round Trips

  • En lisant le code, relevez le moindre détail dans un commentaire de revue dès que vous le voyez, puis arrêtez immédiatement votre lecture
  • Le développeur corrige consciencieusement ce détail et soumet un patch mis à jour
  • Le relecteur commence alors à lire cette nouvelle version et repère un autre détail mineur. Il aurait pu le mentionner dès la première revue, mais ne l'a pas fait. Il signale ce détail et arrête à nouveau sa lecture
  • Répétez jusqu'à ce que le développeur perde tout espoir

The Ransom Note

  • Ce patch semble particulièrement important pour le développeur
  • Mais il ne l'est pas autant pour le relecteur. Le relecteur est donc en position de force
  • Le changement que veut le développeur peut être retenu en otage jusqu'à ce que d'autres tâches, importantes pour le relecteur, soient terminées
    • Ces tâches supplémentaires n'ont pas forcément besoin de faire partie du même commit, mais elles comptent pour le relecteur

The Double Team

  • Un patch, deux relecteurs
  • Chaque fois qu'un relecteur demande un changement, le développeur s'exécute docilement. C'est alors au tour de l'autre relecteur de se plaindre
  • Les relecteurs alternent et formulent des exigences contradictoires
    • Mais ils s'adressent toujours au développeur. Ils évitent de débattre directement entre eux dans le fil de revue
  • L'objectif est de voir combien de fois les relecteurs peuvent renvoyer le développeur d'un côté à l'autre avant qu'il n'abandonne

The Guessing Game

  • Il y a un problème, et plusieurs façons de le résoudre sont possibles
  • Le développeur choisit une solution et soumet un patch
  • Le relecteur critique cette solution précise pour des raisons sans rapport avec le fait qu'elle résolve ou non le problème
    • Par exemple, une violation de principes de conception vagues, ou une incompatibilité avec de futurs plans de développement
  • Si la critique reste suffisamment floue, le développeur ne saura pas comment modifier le patch existant pour y répondre
    • Le mieux pour lui sera alors de choisir une solution totalement différente pour le problème initial et de l'implémenter à la place
  • Et le relecteur la rejettera de nouveau d'une manière tout aussi peu utile

The Priority Inversion

  • Lors de la première revue de code, on signale des choses mineures et simples
  • On attend que le développeur les corrige, puis on soulève un problème majeur
    • Il existe un problème fondamental qui impose de réécrire entièrement une partie importante du patch. Cela signifie qu'une bonne partie des petites corrections déjà effectuées devra être jetée
    • Rien ne transmet mieux le message « votre travail n'est pas souhaité et votre temps n'a pas de valeur » que de faire faire beaucoup de travail à quelqu'un pour ensuite lui faire abandonner ce travail
  • Cela seul peut suffire à pousser le développeur à abandonner

The Late-Breaking Design Review

  • Un travail extrêmement complexe progresse depuis des semaines ou des mois, en de nombreux patchs distincts
  • Le relecteur n'est pas d'accord avec la conception globale de ce travail, mais il n'a pas participé à la discussion initiale ou appartenait au camp qui a perdu le débat
  • Puis le relecteur reçoit une demande d'examen d'une petite mais importante partie située au milieu de ce travail — par exemple, un correctif simple pour un bug qui bloque beaucoup de développeurs. Pour lui, c'est une occasion
  • Le relecteur exige alors une justification de la conception globale de tout le travail réalisé jusqu'ici
  • Si le développeur ne s'occupe que d'une partie de l'ensemble et ne connaît pas la réponse, ce n'est pas le problème du relecteur. Il n'appuiera pas sur le bouton « approuver » tant qu'il ne sera pas satisfait

The Catch-22

  • Si le patch est gros, il est trop difficile à lire. On exige qu'il soit découpé en sous-parties self-contained
  • À l'inverse, s'il y a beaucoup de petits patchs, certains n'ont pas de sens pris isolément. On exige alors de les regrouper
  • Dès qu'un certain type de compromis semble pertinent dans un cas donné, on peut l'exploiter pour trouver un motif de plainte
    • Par exemple, si le code est écrit pour être facile à reconnaître, il sera peu performant ; s'il est bien optimisé, il sera difficile à lire et à maintenir

The Flip Flop

  • Il existe un type de changement que les gens apportent généralement à une même partie du code
  • Le relecteur a déjà examiné plusieurs fois ce genre de changement
  • Un nouveau changement arrive. L'auteur a étudié attentivement les modifications précédentes, suivi soigneusement le modèle existant et choisi un relecteur qui semble bien connaître cette zone
  • Le relecteur conteste soudain un aspect du changement qui n'avait jamais posé problème auparavant. Suivre le modèle existant ne suffit plus
  • Que se passe-t-il si l'auteur montre un changement identique du passé pour souligner l'incohérence du relecteur ?
    • Le relecteur répond : « Oui, ça aussi, il faut le changer. »
    • Le relecteur doit veiller à ne pas se porter volontaire pour modifier lui-même tous les cas existants. Avec un peu de chance, le développeur comprendra cela comme une instruction de modifier lui-même les cas existants, ce qui épargnera beaucoup d'efforts au relecteur

Mais sérieusement...

  • Jusqu'ici, cet article était une blague. Il ne cherche pas à suggérer que les relecteurs adoptent volontairement ce mauvais comportement
    • La plupart des descriptions sont soit des exagérations de ce que font réellement les relecteurs, soit des exagérations de ce qu'un auteur de patch frustré peut ressentir
  • En réalité, il ne faut pas faire cela !
    • Efforcez-vous de minimiser les allers-retours, demandez les réécritures importantes avant de pinailler sur des détails (si nécessaire), et lorsque vous critiquez un patch, faites des propositions constructives sur la version que vous seriez prêt à accepter
    • Si vous avez un avis sur l'ensemble de la base de code, il vaut mieux en discuter globalement avec tous les développeurs plutôt que de chipoter en examinant le patch d'un seul développeur
    • Si vous avez fait cela par inadvertance, prenez-en conscience. Reconnaissez que vous avez rendu la vie d'un développeur plus difficile sans le vouloir, excusez-vous et essayez d'être plus utile
  • Le problème de fond, c'est l'abus de pouvoir
    • Lorsqu'un développeur devient le relecteur du patch d'un autre développeur, il acquiert un pouvoir temporaire. Le relecteur a le pouvoir d'empêcher que ce patch soit intégré
    • Le pouvoir implique des responsabilités. Il faut l'utiliser uniquement dans le but prévu, et seulement dans la mesure nécessaire
    • La plupart des antipatterns (ou des comportements modérés qu'ils tournent en dérision) relèvent d'un abus de pouvoir. Le relecteur utilise comme levier son pouvoir temporaire sur le développeur pour poursuivre des objectifs personnels sans rapport avec l'amélioration du patch, voire contraires à celle-ci
    • Ces objectifs personnels varient selon l'antipattern : travail sans lien, préférences stylistiques personnelles, paresse, résistance au changement, ou antipathie personnelle envers l'auteur du patch
  • Si l'auteur du patch a lui-même eu ce mauvais comportement par le passé lorsqu'il était relecteur de code, cette antipathie est peut-être justifiée. C'est pourquoi il faut user avec prudence du pouvoir du relecteur
    • Si les développeurs tombent dans un cercle vicieux de représailles mutuelles, le projet logiciel est condamné

La revue de code comme gatekeeping

  • Jusqu'ici, on s'est concentré sur la revue de code entre pairs
    • Le relecteur de code et l'auteur du patch sont des collègues ; ils ne sont pas responsables l'un de l'autre et n'ont pas le dernier mot sur la base de code
    • Le pouvoir tiré de la revue de code est donc temporaire
  • Dans une situation de revue entre pairs, le relecteur et l'auteur devraient fondamentalement poursuivre le même objectif
    • S'il existe de profonds désaccords sur les fonctionnalités à intégrer, le niveau de finition requis avant approbation ou le style de code acceptable, il faut les traiter en dehors du cadre de la revue de code
  • Mais dans d'autres types de revue de code, ce n'est pas le cas. En particulier lorsque des personnes extérieures au projet envoient des patchs non sollicités, la situation est très différente
    • Ce scénario se produit généralement dans le logiciel libre
    • Parce que n'importe qui dans le monde peut modifier le code source, et que certains essaient de renvoyer leurs modifications
  • Mais cela peut aussi arriver dans d'autres contextes
    • Dans une entreprise qui développe du code propriétaire, un développeur d'une équipe peut envoyer un patch au projet d'une autre équipe en espérant avoir de la chance
  • Dans ces situations, il est beaucoup plus probable que le destinataire du patch ne veuille pas de ce changement dès le départ, ou soit totalement en désaccord avec sa mise en œuvre, au point de ne pas vouloir l'accepter du tout
    • Dans ce cas, il ne s'agit pas d'un abus du pouvoir temporaire accordé au relecteur pair, mais de l'exercice légitime d'un pouvoir permanent en tant que responsable du projet
    • C'est moi qui décide de l'orientation de mon projet logiciel
  • Quand le relecteur joue ce rôle de « gatekeeping », il n'est pas toujours mauvais de critiquer un patch au motif qu'il viole des principes généraux de conception ou des exigences existantes
    • Il est possible qu'il ne sache tout simplement pas comment résoudre ce problème d'une manière compatible avec ces exigences
    • En réalité, c'est peut-être justement la partie difficile, et la seule raison pour laquelle lui-même n'a pas encore apporté exactement le même changement
  • Cependant, même dans un contexte de gatekeeping, appliquer « The Guessing Game » sans explication reste impoli
    • J'essaie généralement d'expliquer que le développeur a négligé la partie difficile et, si je n'ai pas moi-même la réponse, je le dis
  • Bien sûr, rien ne peut excuser une obstruction passive-agressive comme « The Death of a Thousand Round Trips »
    • Si vous ne voulez vraiment pas du tout intégrer le patch dans le code et que vous avez la légitimité pour prendre cette décision en tant que gardien du projet, vous pouvez le dire clairement afin que l'auteur ne perde pas davantage son temps

Disclaimer

  • J'ai rassemblé pendant des années des notes pour cet article à partir de revues de code auxquelles j'ai participé (dans les deux rôles), de revues de code observées entre d'autres personnes, et de revues de code dont j'ai seulement entendu parler en conversation
  • Cet article ne vise donc pas une personne en particulier qui aurait récemment relu mon code

13 commentaires

 
intajon 2025-02-17

Ce n’est peut-être pas exagéré, finalement....

 
magulhalmom 2024-09-02

Je l’ai vraiment vécu, et j’ai failli arrêter d’être développeur. Franchement, ça m’a été très difficile de me relever.

 
bungker 2024-08-28

J’ai failli avoir un PTSD en lisant l’article.

 
kayws426 2024-08-28

On dirait que vous avez bien rassemblé vos notes pour cet article depuis tout ce temps !!

 
gongsil1212 2024-08-27

Rien qu’à les lire, on est au niveau de la maltraitance mentale...

 
wedding 2024-08-27

Donc, l’essentiel est dans la dernière ligne, non ? mdr.....

 
jypark 2024-08-27

Waouh… j'ai cru que j'allais en mourir de stress ;;

 
ahwjdekf 2024-08-26

Ce genre de chose, si vous allez dans des endroits où l’on fait du SI + SM, vous en verrez souvent aussi en Corée. On appelle souvent ça un esprit de clan. Des gens malveillants font toutes sortes de coups pour protéger leur pré carré.

 
kenuheo 2024-08-26

Il y a beaucoup de méthodes diaboliques.

 
gooksangom 2024-08-26

À long terme, les personnes qui se comportent ainsi sans raison valable finissent soit par 1) être exclues très tôt du réseau professionnel des développeurs, soit, si malgré un caractère exécrable elles ont des compétences exceptionnelles, par se voir confier une grosse part du travail et devenir difficiles à écarter ; dans ce cas, la situation ne tient que parce que quelqu’un joue le rôle d’adaptateur et de canal de communication pour maintenir le lien, et si cet intermédiaire disparaît pour une raison ou une autre, elles ne tardent pas à être exclues. On aura beau se démener autant qu’on veut, au final il faut bien que des gens se rassemblent pour faire quelque chose afin que l’argent circule, et c’est parce que l’argent circule que les gens circulent aussi ; quelqu’un qui ne sait pas bien s’entendre avec les autres finit donc forcément par être exclu du groupe et mis de côté.

En général, les personnes au sale caractère qui survivent longtemps dans un groupe se trompent souvent en croyant qu’elles ont tenu parce qu’elles seraient une sorte d’être exceptionnel, comme un Sherlock de drama, un grand « sociopathe à haut fonctionnement ». En réalité, si on les supporte autour d’elles, c’est uniquement parce qu’elles ont encore une utilité ; dès que cette utilité disparaît, la relation devient du genre « c’était pénible de t’avoir avec nous, et ne nous revoyons plus jamais ». Même le Sherlock incarné par Cumberbatch n’est, de notre point de vue extérieur, qu’un sociopathe charismatique ; s’il n’y avait pas eu autour de lui des gens qui refusaient de l’abandonner et tenaient à lui, il n’y aurait tout simplement pas eu d’histoire.

 
dokdok 2024-08-28

Les personnes au caractère détestable qui ont malgré tout réussi à durer longtemps ont souvent tendance à se persuader qu’elles ont survécu parce qu’elles seraient quelque chose d’exceptionnel, une sorte de sociopathe hautement fonctionnel tout droit sorti de Sherlock ou d’un drama. Mais en réalité, c’est simplement que leur entourage les supporte en serrant les dents tant qu’elles restent utiles ; dès que cette utilité disparaît, la relation devient du genre : « C’était pénible de te côtoyer, et j’espère ne plus jamais te revoir. » ==> C’est une formule incroyable. Je vais m’en souvenir.

 
savvykang 2024-08-26

Ça me fait penser au harcèlement au travail, voire au power harassment.

 
xguru 2024-08-26

C’est censé être de l’humour, mais rien qu’en lisant l’article, l’agacement monte d’un coup..