(PR) Ce niveau d'exigence est-il fréquent en entreprise ?

Bonjour,

A l’approche de mon entretien annuel, j’ai entendu dire que ma rigueur concernant la qualité du code doit être améliorée (reproche qui m’avait été fait il y a quelques mois aussi). Pourtant, depuis que je travaille pour ma boîte actuelle, j’observe cela (moins vrai au tout début) :

• les PR contiennent entre 0 et grand max 10 retours, le plus souvent uniquement 4 ou 5 retours.

• je les traite entre 10min et 1h grand max, le plus souvent en 30min / 45min.

• D’après les commentaires GitHub, Slack et oraux de mon Lead Dev et de mon CTO : il s’agit de détails, ils qualifient quasi-systématiquement leurs retours ainsi, ajoutant parfois des trucs du style "rien de lourd ne t’inquiète pas".

• De mon point de vue, je confirme cela : toutes PR confondues, 80% sont de l’ordre du détail, le reste est 90% du temps très facilement corrigeable sans que ce soit du détail, mais c’est pas des trucs très importants. Il est arrivé, historiquement sur une période de plus d’un an, que sur deux ou trois PR max, il faille reprendre tout le code MAIS c’était justifié car le besoin produit n’avait pas été correctement spéqué, et que le nouveau besoin avait dû être reclarifié entre le CTO et le PO pour qu’on me redise quoi faire (j’étais impliqué dans leur conversation mais ne participais pas car j’ai du mal avec les définitions des nouveaux besoins produits, voire avec leur implémentation technique quand c’est à l’oral en direct => j’ai besoin de réflexion asynchrone).

• Exemples de détails :

  • Raccourcir les noms de variables. Du genre j’écrivais $pendingTransactionLineAmount, on m’a demandé de raccourcir : $amount, ou raccourcir $pendingTransactionLines en $tLs.

    • Cela peut prêter à sourire, mais soit disant que j’aurais mis 3 mois à raccourcir comme demandé mes noms de variables d’après le CTO (faux. Je l’ai fait sous peu dès ses premiers retours en ce sens, avec juste quelques oublis concernant certaines variables !). C’était un sujet important qui a fait l’objet d’une phrase dite par mon CTO avec un ton énervé du genre "Non mais ça fait 3 mois que je te demande de raccourcir tes fiches noms de variables ! ". De mon point de vue, sa demande n’est même pas bonne (préférer des acronymes à des noms explicites, bof bof, mais je fais comme il dit pour ne pas faire de remous, surtout que le Lead Dev est d’accord avec lui).

  • Refactorer des trucs que j’écris en 6–8 lignes en 3–4 lignes. Mais la logique était OK.

  • Une fois : mon PHPStorm (IDE) bug je pense, j’avais utilisé une instance du package Carbon d’un autre namespace que celui déjà utilisé dans le code, évidemment j’ai dû refactorer ça mais j’avais pas vu.

• Exemples de trucs qui, à mes yeux, ne sont pas des détails :

  • Réécrire des routes du genre POST /recettes/recette1/melangerAliment en POST /recettes/recette1/aliments/aliment1/melanger pour bien respecter REST

  • Je n’ai pas d’autres trucs en tête.

La tendance

Depuis plusieurs mois, le nombre de retours est encore plus petit qu’avant, voire 0 retours, on merge direct. Au max je dirais 5 retours, mais 1 à 3 retours semble être le plus grand nombre.

Objectifs demandés par le CEO et le CTO

• C’est de nouveau le CTO qui review mes PR. Le Lead ne le fait plus car on le noie avec plein de travail, je crois. Avant la venue du Lead il y a 6 mois : c’était aussi le CTO qui reviewait mon travail (sauf pendant plusieurs mois car il était débordé). Durant les 6 premiers mois du lead : c’était souvent le Lead qui reviewait mon travail.

• Les clients grossissent => plus de responsabilités envers eux et entre nous => demande de réduction considérable des retours PR (= demande de plus de qualité de la part des devs).

• L’équipe va s’agrandir => le CTO aimerait ne plus devoir review mes PR et qu’on merge direct sans review, surtout pour les plus simples mais ce serait généralisable à presque toutes les PR à terme, afin que je gagne en autonomie. (Mais du coup je ne comprends pas à quoi sert cette étape de PR mais bon…).

• Si j’appliquais ce que le CTO souhaite aux faits actuels, je passerais donc d’une moyenne de 45min de correction pour appliquer ses retours PR à 10min max, et d’un nombre déjà faible de retours du genre 5–6 retours à 1–2 retours, non ?

Question

Cela vous semble-t-il cohérent, normal, pour une entreprise (ici une startup) ? Ne s’agit-il pas d’exigences démesurées ?

Je me demande si le CTO observe les mêmes faits statistiques que moi………………. Et comment il peut qualifier de "détails/retours PR pas lourds" mon travail, tout comme le Lead, pour ensuite me faire ce genre de remarques.

Témoignages dans mon cercle de connaissances

Vu ce qu’on me dit (potes de fac, connaissances sur Internet), j’ai l’impression que mon travail est très quali justement. Ce qui ne va visiblement pas dans le sens de la pensée du CTO…

Conclusion

Si je devais conclure ce que je pense de la situation.

Le CTO vit dans une autre réalité, est incohérent avec lui-même et avec le Lead au niveau des qualificatifs utilisés minimisant la quantité et l’importance de leurs retours PR ; il n’a pas les mêmes faits / statistiques que ceux que je constate de mon côté au niveau des retours PR.

Clairement non.
Vouloir bypasser le processus de pull-request pour gagner du temps, c’est se tirer une balle dans le pied.

Ils s’en rendront compte le jour où un bug important sera introduit, qui aurait pu être évité par une bonne revue, et qu’ils y perdront plus de temps que ce qui aurait été nécessaire à l’éviter.

Mais pour qu’une revue soit utile, il faut qu’elle soit constructive (sur le fond) et pas juste là pour faire semblant.

Ta dernière phrase fait allusion au raccourcissement des noms de variables par exemple ou au refacto de plusieurs lignes en un nombre de lignes plus petit, mais à logique équivalente ?

Si oui, en fait le CTO fait ça afin que l’on ait une codebase hyper saine et hyper lisible, bien écrite , facile à lire. C’est une énorme composante dans la qualité du code à ses yeux.

En fait de la façon dont tu présentes les choses, j’ai l’impression qu’actuellement ils ont un processus de review juste pour se dire qu’ils en ont un, pour « faire bien ».
Mais qu’ils n’y voient pas plus d’intérêt que cela et n’en tirent pas les avantages qu’ils devraient (vérification des comportements introduits, retours et suggestions sur le code, performances des requêtes et/ou algorithmes, cohérence des modifications avec la codebase) se contentant de quelques commentaires cosmétiques.

Mais qu’ils n’y voient pas plus d’intérêt que cela et n’en tirent pas les avantages qu’ils devraient (vérification des comportements introduits, retours et suggestions sur le code, performances des requêtes et/ou algorithmes, cohérence des modifications avec la codebase) se contentant de quelques commentaires cosmétiques.

Si si tout ça ça fait partie des PR.

Mais juste, me concernant, c’est 80% du temps des détails comme ils disent eux-mêmes, c’est ça le souci. Et malgré cet état de fait, vlà-t’y pas qu’on me reproche la qualité du code, c’est à s’en mordre la queue.

Par ailleurs tu fais bien de parler des perfs : y a longtemps, il m’avait été reproché, à raison, de ne pas vérifier les perfs avec un outil externe, ce que je fais désormais. C’était il y a plusieurs mois. Il en était de même pour l’autre dev avec moi.

Aujourd’hui en faisant le tour de l’app juste avant les MEP, ils se rendent compte qu’il y a des soucis de perfs à certains endroits. Et on reproche à l’équipe (et notamment à moi) ces soucis de perfs. Le souci c’est que, pour en avoir vérifié certains, le code ne venait pas de moi et, de plus, dans mes sprints récents, en aucun cas je n’avais besoin de parcourir l’app pour vérifier les perfs. Du coup ça m’agace un peu mais bon.

Les peer-reviews sont importantes dans le processus qualité pas seulement pour corriger la qualité du code, mais aussi pour limiter les erreurs humaines, partager la maitrise du sujet, et partager des compétences en apprenant de la relecture ou des remarques.
Les remarques qui te sont faites tiennent de l’ordre du coding guidelines. Il faudrait que ce soit formalisé dans un document.

Mais je trouve surtout que ton CTO qui "adresse des reproches" tout comme toi qui cherche à monter des argumentaires de défense ne comprenez pas ce que c’est de travailler en équipe. La responsabilité est partagée, il faut arrêter d’essayer de trouver des coupables. L’exemple des perfs, il faut mettre en place des métriques, avec idéalement des outils automatisés, et corriger avec éventuellement du refactor. ça va bien être à toi de réaliser ces tâches mais ça va être au manager de mesurer et planifier la charge pour te laisser les exécuter, il faut qu’il définisse les priorité. Donc si des problèmes de perfs apparaissent c’est autant la faute du codeur qui les a introduit, que du relecteur qui ne les as pas vu, que du testeur qui ne les a pas détecté, que du manager qui n’a pas donné de prio aux perfs. C’est la faute de tout le monde, autrement dit la responsabilité est partagée à toute l’équipe. Maintenant il faut raisonner par constat et action.

Salut,

Pour faire court, je ne vis pas dans le même monde que toi. On a des anciens sur le projet (> 2ans), et ils ont quand même droit à des relectures. Et on trouve des choses. Et on en laisse passer…

C’est comme quand je valide sur ZdS, même avec un auteur qui a de l’expérience et qui fait les choses bien, il y a des trucs à peaufiner.

En vrac,

• un nom de variable qui est une abréviation non standard, ou un truc trop flou même dans le contexte (« quantité », mais quantité de quoi), c’est de mon point de vue une mauvaise pratique ; on veut que tu fasses comme ça, soit ;
• des POST /recettes/recette1/aliment1/melanger, soit c’est dans la spec, et tu aurais dû la lire, soit ce n’est pas dedans, et tu aurais certes dû proposer mieux, mais ce n’est pas à toi d’inventer dans ton coin une API, c’est à l’archi de faire ça ;
• « le CTO aimerait ne plus devoir review mes PR et qu’on merge direct sans review » Je ne vois pas ce qui pourrait mal se passer ⸮

Bref, le niveau d’exigence du CTO me semble très légère envers ses processus, et il reporte ça sur le dév, en tout cas de ce que tu présentes. Le fait qu’on découvre des problèmes de perf tard va dans le même sens. Et le fait de chercher des coupables plutôt que des solutions est une alerte rouge pour moi.

Un bon dév dans une équipe n’est pas quelqu’un qui ne fait pas d’erreur, c’est quelqu’un qui se débrouille pour voir ses erreurs et celles des autres avant qu’elles n’aient des conséquences. Donc relecture, test, linter, analyse, etc.

"rigueur concernant la qualité du code doit être améliorée (reproche qui m’avait été fait il y a quelques mois aussi)."

J’espère qu’ils viennent avec des exemples concrets, parce tel quel c’est bien trop vague pour que tu puisses en faire qq chose.

Les reproches concernant les noms des variables sont ridicules, je comprends pas qu’ils te reprochent ça.

De plus, Je ne vois pas le rapport entre peer review et autonomie. Ce n’est pas parce que tu es autonome que tu ne dois pas être peer reviewed.

Il suffit que tu sois un peu fatigué un jour, pour faire une erreur de codage… ca arrive à tout le monde, même aux meilleurs. Le peer reviewing sert justement à éviter des erreurs d’inattention, entre autre. Quand on fait du Peer review, on le fait pour tout le monde, pas uniquement pour les petits nouveaux. Même Linus torvald se fait peer reviewer (enfin je suppose, j’ai pas été vérifier :D).

Bref ils te reprochent un manque de rigueur, mais font des remarques qui n’ont pas lieu d’être sur des noms de variables et considèrent que le PR comme accessoire. Cohérence, cohérence…

C’est moi ou le management a l’air un peu foireux dans cette boîte. Ça doit être la folle ambiance chez vous dis donc, si à chaque fois qu’il y’a un problème, ils cherchent un coupable

J’ai toujours la même question, mais… c’est toujours la même boite ?

Si oui, même remarque que dans tes ~36 sujets précédents, hein.

Bonsoir à tous,

Attention, c’est les PR les plus simples qui pour le moment seraient concernées par cette absence de review. Mais oui ça serait généralisé à la plupart des reviews à venir, mais pas toutes.

Bon bah sinon je n’ai rien à ajouter, je suis d’accord avec vous.

Je me demande vraiment ce qu’il va être attendu de moi, sachant que je fais déjà peu d’erreurs occasionnant de réels retours et que les retours sont traités rapidement… Mais bon…

Un autre point qui a été soulevé concerne cette fois-ci les tests : il a été dit qu’il faudrait que le PO, qui est un peu le QA, teste et valide direct. Or ce n’est pas encore le cas (pas forcément me concernant hein, moi au contraire en général c’est OK du premier coup). En soi pourquoi pas, c’est normal d’exiger des devs qu’ils testent tout de leur côté. Là où ça devient embêtant c’est de fixer comme objectif "le PO doit systématiquement pouvoir être capable de tester et valider direct" ===> bah non, y a des fois où malgré tout le dev aura commis une erreur, ou alors souci en amont (specs, communication etc). Le souci est que la faute va aller au dev en mode 0 tolérance.

Une illustration de cela dont je vous avais déjà parlée : un jour le PO s’est aperçu que le front n’appelait pas bien mon API. On m’a reproché de ne pas avoir testé tout le process sur l’application (à juste titre ou pas, vous aviez des avis divergents là-dessus mais pour la majorité d’entre vous c’était normal et de mon point de vue aussi). Le souci est que finalement on n’a quasiment rien reproché au dev front ni au reviewer des PR front (le CTO) ! C’est moi qu’on a responsabilisé à 100% ou presque. Alors qu’à la base, quoi qu’on en dise, j’suis dev back quand même quoi, moi j’avais testé en tests automatisés et sur postman (alors ok pas sur l’app et c’est pas bien mais voilà, j’avais fait en très grande partie mon job).

Ceci dit non, dans ma boîte on évite de désigner tel ou tel personne comme "coupable" d’une erreur, du moins officiellement et c’est ce qui est rappelé par le CTO fréquemment. Il essaie toujours de parler au nom de l’équipe, donc ça c’est cool.

Ceci dit non, dans ma boîte on évite de désigner tel ou tel personne comme "coupable" d’une erreur, du moins officiellement et c’est ce qui est rappelé par le CTO fréquemment.

Ben alors comment ça se fait que tu as l’impression qu’on te reproche tout plein de trucs personnellement ? Soit tu psychotes pour rien, soit ils sont pas si bons que ça à éviter de désigner des personnes directement, soit c’est un bon mélange bien malsain des deux…

Par ailleurs, en ce qui concerne

j’ai entendu dire que ma rigueur concernant la qualité du code doit être améliorée (reproche qui m’avait été fait il y a quelques mois aussi). Pourtant, depuis que je travaille pour ma boîte actuelle, j’observe cela (moins vrai au tout début) : […]

Le fait que tu as peu de remarques sur tes PR n’est pas forcément incompatible avec le fait qu’on te demande plus de rigueur. Si les remarques que tu reçois sont souvent de même nature et/ou se résument au fait que t’as pas fait gaffe à tel ou tel détail, c’est effectivement un manque de rigueur de ta part. Est-ce que c’est grave ? Probablement pas. Est-ce qu’il est possible et souhaitable de t’améliorer sur ces points ? Probablement, et on peut imagine que c’est dans ce sens que la remarque allait. Maintenant, vue l’ambiance complètement pourrie que tu décris par ailleurs à travers tes messages, ça peut aussi être une remarque qui n’est pas fondée et/ou ne mérite pas ta considération. Pour être franc, c’est même la boite entière qui n’a pas l’air de mériter de considérations.

Pour moi, ces deux affirmations issues du même message sont contradictoires.

Bonsoir,

Décidément, elle est vraiment bizarre ta boîte, si c’est toujours la même.

Une MR doit toujours être revue par au moins une personne, et même plusieurs pour les MR plus conséquentes. Même si c’est le tech lead qui l’a développée. Même si tu as 10 ans d’ancienneté. Même si tu es une rockstar du code. Même si le projet est en retard.

Chez nous, même en ayant des développeurs, des revues, des tests automatisés, des tests QA, des tests UAT, et des test pré-production, il y a encore parfois des erreurs qui passent à travers et qui se retrouvent jusqu’en production. Il y a de tout: des erreurs métier parce que quelqu’un n’a pas bien compris le besoin à un moment donné, et des erreurs techniques plus ou moins stupides ou plus ou moins impactantes.

Le but ce n’est pas de ne plus faire aucune erreur, ça ça n’arrivera jamais, mais c’est de les repérer le plus tôt possible pour qu’elles aient le moins de conséquences possibles.

Supprimer ou négliger n’importe quelle étapes va dans un premier temps permettre de faire des économies, mais seulement jusqu’au jour où tout explosera. Et là vous allez payer x10, ou peut-être encore bien plus. Vous n’allez peut-être pas non plus payer rien qu’en argent, la réputation ça peut coûter aussi très cher.

JE ne suis pas manager ni tech lead moi-même, mais j’estime que si l’un ou l’autre ne peut pas comprendre ça, je suis désolé, il ne mérite pas sa place.

Maintenant pour les points que tu cites comme détail, ça n’est pas tant des détails que ça. Ce qui me surprend, c’est que ton chef va à l’encontre des bonnes pratiques habituelles.

Les redondances inutiles dans le nommage de classes, méthodes et variables devraient certes être évités, mais sinon de manière générale, mieux vaut choisir des noms assez explicites. Est-ce que dans 6 mois, quand tu reliras ton code, tu te souviendras de ce que signifie $tLs ? Est-ce que le collègue qui doit mettre les pieds dans ton code va savoir, lui ?

Et pour ce qui est de la longueur du code, moins de lignes ne signifie pas forcément mieux. Ca dépend des cas.

Deux points que l’on a tendence à oublier sur l’intérêt des MR :

1. Tout le monde fait des coquilles. Par exemple, aujourd’hui au boulot on a perdu une demi-journée avant de se rendre compte qu’un paramètre s’appelait machin_enabled et pas machin_enable.
2. Relire une MR, c’est aussi partager la connaissance. C’est être au moins 2 avoir une idée – même vague – de ce que fait chaque ligne du code. Ça veut dire qu’en cas d’indisponibilité d’une personne, il y aura toujours quelqu’un d’autre capable d’aller corriger une urgence sans avoir besoin de totalement découvrir le truc (je parle bien entendu à équipe constante). Ça veut dire aussi qu’il n’y a pas, dans la base de code, de code compréhensible par une seule personne.

Ces deux points, seuls, suffisent à justifier les MR, même quand c’est un débutant qui relit les MR d’un rockstar senior architecte en chef.

Salut,

Moi, au milieu de tout ça, y a un truc qui me choque. Et ce truc, c’est ça :

Cela vous semble-t-il cohérent, normal, pour une entreprise (ici une startup) ? Ne s’agit-il pas d’exigences démesurées ?

J’espère sincèrement que ce n’est pas une startup, mais que tu emploies ce terme par abus de langage.

Parce que sinon, je plains celles et ceux qui y travaillent et qui y ont mis des billes.