Après de graves problèmes de qualité au cours de la dernière année, mon entreprise a récemment introduit la révision du code. Le processus de révision du code a été introduit rapidement, sans lignes directrices ni liste de vérification.

Un autre développeur et moi avons été choisis pour passer en revue toutes les modifications apportées aux systèmes avant leur fusion dans le coffre.

Nous avons également été choisis comme "responsable technique". Cela signifie que nous sommes responsables de la qualité du code, mais nous n'avons aucune autorité pour mettre en œuvre des modifications dans le processus, réaffecter des développeurs ou retenir des projets.

Techniquement, nous pouvons nier la fusion en la rendant au développement. En réalité, cela se termine presque toujours avec notre chef qui exige qu’il soit expédié à temps.

Notre responsable est un MBA principalement chargé de créer un calendrier des projets à venir. Pendant qu’il essaie, il n’a pratiquement aucune idée de ce que notre logiciel fait du point de vue commercial et a du mal à comprendre même les demandes les plus élémentaires de ses clients sans les explications d’un développeur.

Actuellement, le développement est effectué dans les branches de développement de SVN, après que le développeur se soit dit prêt, il réaffecte le ticket de notre système de ticketing à notre responsable. Le responsable nous le cède ensuite.

Les revues de code ont créé des tensions au sein de notre équipe. En particulier, certains des membres les plus âgés remettent en cause les changements ("Nous l’avons toujours fait comme ça" ou "Pourquoi la méthode devrait-elle avoir un nom raisonnable, je sais ce qu’elle fait?").

Après les premières semaines, ma collègue a commencé à laisser les choses glisser pour ne pas causer de problèmes avec les collègues (elle m'a dit elle-même qu'après qu'un rapport de bogue avait été signalé par un client, elle était au courant du bogue, mais craignait que le développeur serait en colère contre elle pour le signaler).

De mon côté, je suis maintenant connu pour être un âne pour signaler des problèmes avec le code engagé.

Je ne pense pas que mes normes sont trop élevées.

Ma liste de contrôle en ce moment est:

• Le code va compiler.
• Il y a au moins une façon dont le code fonctionnera.
• Le code fonctionnera avec la plupart des cas normaux.
• Le code fonctionnera avec la plupart des cas extrêmes.
• Le code lève une exception raisonnable si les données insérées ne sont pas valides.

Mais j'accepte pleinement la responsabilité de la façon dont je donne des commentaires. Je donne déjà des points explicables expliquant pourquoi quelque chose devrait être changé, parfois même demandant simplement pourquoi quelque chose a été mis en œuvre de manière spécifique. Quand je pense que c'est mauvais, je souligne que je l'aurais développé d'une autre manière.

Ce qui me manque, c'est la capacité de trouver quelque chose à qualifier de «bon». J'ai lu qu'on devrait essayer de prendre entre mauvaises nouvelles et bonnes nouvelles.

Mais j'ai du mal à trouver quelque chose de bien. "Hé cette fois, vous avez réellement commis tout ce que vous avez fait" est plus condescendant que gentil ou utile.

Exemple de révision de code

Salut Joe,

J'ai quelques questions sur vos modifications dans la classe Library \ ACME \ ExtractOrderMail.

Je ne comprenais pas pourquoi vous avez marqué "TempFilesToDelete" comme statique? Pour le moment, un deuxième appel à "GetMails" lève une exception, car vous y ajoutez des fichiers sans jamais les supprimer, après les avoir supprimés. Je sais que la fonction n'est appelée qu'une fois par exécution, mais cela pourrait changer à l'avenir. Pourriez-vous simplement en faire une variable d'instance, nous pourrions alors avoir plusieurs objets en parallèle.

... (Quelques autres points qui ne fonctionnent pas)

Points mineurs:

• Pourquoi "GetErrorMailBody" prend-il une exception en tant que paramètre? Ai-je oublié quelque chose? Vous ne lancez pas l'exception, il vous suffit de la transmettre et d'appeler "ToString". Pourquoi donc?
• SaveAndSend n'est pas un bon nom pour la méthode. Cette méthode envoie des mails d’erreur si le traitement d’un mail a mal tourné. Pourriez-vous le renommer "SendErrorMail" ou quelque chose de similaire?
• S'il vous plaît, ne faites pas que commenter l'ancien code, supprimez-le immédiatement. Nous l'avons toujours en subversion.

Comment trouver des choses positives dans une revue de code?

Après de graves problèmes de qualité au cours de la dernière année, mon entreprise a récemment introduit la révision du code.

Génial, vous avez une réelle opportunité de créer de la valeur pour votre entreprise.

Après les premières semaines, ma collègue a commencé à laisser les choses glisser pour ne pas causer de problèmes avec les collègues (elle m'a dit elle-même qu'après un rapport de bug établi par un client, elle était au courant du bug, mais craignait que le développeur serait en colère contre elle pour le signaler).

Votre collègue ne devrait pas procéder à la révision du code si elle ne parvient pas à expliquer aux développeurs ce qui ne va pas avec leur code. Il vous incombe de détecter les problèmes et de les résoudre avant qu’ils n’affectent les clients.

De même, un développeur qui intimide ses collègues demande à être licencié. Je me suis senti intimidé après une révision du code - j'ai prévenu mon patron, et tout a été traité. De plus, j'aime mon travail et j'ai donc gardé les retours, positifs et négatifs. En tant que critique, c'est sur moi, pas quelqu'un d'autre.

De mon côté, je suis maintenant connu pour être un âne pour signaler des problèmes avec le code engagé.

C'est regrettable, vous dites que vous faites preuve de tact. Vous pouvez trouver plus d'éloges, si vous avez plus à chercher.

Critique le code, pas l'auteur

Vous donnez un exemple:

J'ai quelques questions sur vos changements dans

Évitez d’utiliser les mots "vous" et "votre", dites "le" change à la place.

Ai-je oublié quelque chose? [...] Pourquoi donc?

N'ajoutez pas de fioritures rhétoriques à vos critiques. Ne plaisante pas non plus. J'ai entendu une règle: "Si vous vous sentez bien de dire, ne le dites pas, ce n'est pas bon."

Peut-être que vous améliorez votre propre ego aux dépens de quelqu'un d'autre. Gardez-le juste les faits.

Élevez la barre en donnant des commentaires positifs

Il met la barre plus haut pour féliciter vos collègues développeurs quand ils répondent à des normes plus élevées. Donc, cela signifie que la question,

Comment trouver des choses positives dans une revue de code?

est un bon, et mérite d'être adressé.

Vous pouvez indiquer où le code correspond aux idéaux des pratiques de codage de niveau supérieur.

Cherchez-les à suivre les meilleures pratiques et à continuer à élever la barre. Une fois que les idéaux les plus faciles sont attendus de la part de tous, arrêtez de les louer et cherchez des méthodes de codage encore meilleures.

Meilleures pratiques spécifiques aux langues

Si le langage prend en charge la documentation sous forme de code, d'espaces de nommage, de fonctions de programmation orientée objet ou fonctionnelle, vous pouvez les appeler et féliciter l'auteur de les avoir utilisées, le cas échéant. Ces questions relèvent généralement des guides de style:

• Répond-il aux normes internes du guide de style?
• Répond-il au guide de style le plus reconnu en ce qui concerne la langue (qui est probablement plus stricte qu’en interne - et donc conforme au style en interne)?

Meilleures pratiques génériques

Vous pouvez trouver des points à louer sur les principes de codage génériques, sous différents paradigmes. Par exemple, ont-ils de bons résultats? Les unittests couvrent-ils la majeure partie du code?

Chercher:

• tests unitaires qui testent uniquement la fonctionnalité en question, ce qui constitue une fonctionnalité coûteuse qui n’est pas destinée à être testée.
• niveaux élevés de couverture de code, avec des tests complets des API et des fonctionnalités sémantiquement publiques.
• tests d'acceptation et de fumée qui testent les fonctionnalités de bout en bout, y compris les fonctionnalités simulées pour les tests unitaires.
• bonne dénomination, points de données canoniques pour que le code soit DRY (ne vous répétez pas), pas de chaînes magiques ni de chiffres.
• nommer les variables si bien fait que les commentaires sont en grande partie redondants.
• des nettoyages, des améliorations objectives (sans compromis) et des modifications appropriées qui réduisent les lignes de code et la dette technique sans rendre le code complètement étranger aux auteurs originaux.

Programmation fonctionnelle

Si le langage est fonctionnel ou supporte le paradigme fonctionnel, recherchez ces idéaux:

• éviter les globals et l'état global
• utiliser des fermetures et des fonctions partielles
• petites fonctions avec des noms lisibles, corrects et descriptifs
• points de sortie uniques, minimisant le nombre d'arguments

Programmation Orientée Objet (POO)

Si la langue prend en charge la POO, vous pouvez louer l’utilisation appropriée de ces fonctionnalités:

• encapsulation - fournit une petite interface publique clairement définie et masque les détails.
• héritage - code réutilisé de manière appropriée, peut-être par mixins.
• polymorphisme - les interfaces sont définies, peut-être des classes de base abstraites, des fonctions écrites pour prendre en charge le polymorphisme paramétrique.

sous POO, il existe également des principes SOLID (peut-être une redondance des fonctionnalités de POO):

• responsabilité unique - chaque objet a un acteur / propriétaire
• ouvert / fermé - ne modifie pas l'interface d'objets établis
• Substitution de Liskov - des sous-classes peuvent être substituées aux instances de parents
• ségrégation d'interfaces - interfaces fournies par la composition, peut-être mixins
• inversion de dépendance - interfaces définies - polymorphisme ...

Principes de programmation Unix :

Les principes Unix sont la modularité, la clarté, la composition, la séparation, la simplicité, la parcimonie, la transparence, la robustesse, la représentation, la moindre surprise, le silence, la réparation, l'économie, la génération, l'optimisation, la diversité et l'extensibilité.

En général, ces principes peuvent être appliqués sous de nombreux paradigmes.

Vos critères

Ce sont beaucoup trop triviales - je me sentirais condescendé à si loué pour cela:

• Le code va compiler.
• Il y a au moins une façon dont le code fonctionnera.
• Le code fonctionnera avec la plupart des cas normaux.

D'un autre côté, ces éloges sont assez élogieux, compte tenu de ce que vous semblez avoir à faire, et je n'hésiterais pas à en féliciter les développeurs:

• Le code fonctionnera avec la plupart des cas extrêmes.
• Le code lève une exception raisonnable si les données insérées ne sont pas valides.

Écrire des règles pour passer une révision de code?

C'est une bonne idée en théorie, cependant, bien que je ne rejette généralement pas le code pour une mauvaise désignation, je l'ai tellement nommé que je rejetterais le code avec des instructions pour le corriger. Vous devez pouvoir rejeter le code pour une raison quelconque.

La seule règle à laquelle je puisse penser pour rejeter le code est qu'il n'y a rien d'aussi flagrant que je le garderais en dehors de la production. Je serais bien disposé à ne pas produire de mauvais nom, mais vous ne pouvez pas en faire une règle.

Conclusion

Vous pouvez louer les meilleures pratiques suivies sous plusieurs paradigmes, et probablement sous tous, si le langage les prend en charge.

Ne prenez pas la peine de choisir quelque chose de bien, à moins que ce soit un exemple solide et concis et qu’il soit directement lié au problème ciblé.

Je ne ferai pas de bruit - à première vue, vous avez affaire à au moins une personne qui manque de sécurité à propos de ses capacités et qui est confrontée au défi que lui pose son travail de manière immature. Ils sont probablement aussi mauvais au travail - un bon développeur doit toujours être prêt à se refléter, à accepter les critiques constructives et à être ouvert au changement.

Maintenant que c'est dans l'air, parlons de vous. Peu importe si vous pensez être raisonnable, vous devez faire très attention avec des personnes comme celle-ci pour lancer le processus. J'ai trouvé que le meilleur moyen de traiter avec ces personnes est de faire très attention à la façon dont vous formulez les choses.

Assurez-vous que vous blâmez le code et non l'auteur . Concentrez-vous sur le problème en question plutôt que sur la montagne de caca qui constitue votre base de code, qu'ils ont peut-être eu un rôle important à jouer dans la création et qui serait perçu comme une attaque personnelle supplémentaire. Choisissez d'abord vos batailles, concentrez-vous sur des problèmes critiques qui se manifestent auprès de vos utilisateurs afin de ne pas lancer une volée de critiques à l'encontre de la personne qui les a conduits à rejeter tout ce que vous dites.

Le langage corporel et le ton sont importants si vous leur parlez en personne, expliquez clairement ce que vous dites et assurez-vous de ne pas leur parler de manière neutre ni négliger leurs capacités techniques. Ils seront probablement sur la défensive dès le départ, vous devrez donc régler leurs problèmes au lieu de les confirmer. Vous devez être conscient de cette conversation sans être trop évident afin qu'ils pensent, inconsciemment, que vous êtes de leur côté, et espérons qu'ils acceptent la nécessité d'apporter les modifications qui ont été portées à leur attention.

Si cela ne fonctionne pas, vous pouvez devenir un peu plus agressif. Si le produit est exécutable depuis une salle de conférence, affichez-le sur le projecteur lors de la révision du code et montrez-le directement. C'est mieux si un responsable est présent afin que la personne ne puisse pas revenir en arrière. Ce n'est pas pour leur faire honte, mais pour les forcer à accepter que le problème est réel pour l'utilisateur et qu'il doit être corrigé, au lieu d'être simplement un problème que vous avez avec leur code.

Si vous ne parvenez à rien, si vous êtes fatigué de traiter la personne comme un élève de jardin d'enfants, la direction ignore totalement le problème et cela a une incidence négative sur votre performance en tant que réviseur de code ou sur le bien-être de votre enfant. entreprise et / ou produit, vous devez commencer à parler à votre patron de leur comportement. Soyez aussi spécifique et impersonnel que possible - montrez que la productivité et la qualité en souffrent.

Les révisions de code peuvent être toxiques, perdre du temps, être une source de préoccupation pour les guerres de nerd. Il suffit de regarder la divergence d’opinion sur des choses telles que le code épuré par rapport aux commentaires, les conventions de dénomination, les tests unitaires et d’intégration, les stratégies d’enregistrement, la restitution, etc., etc.

Le seul moyen d'éviter ce problème est d' écrire les règles permettant de passer une révision de code.

Ensuite, ce n'est pas une personne qui échoue ou approuve l'enregistrement. Ils vérifient simplement que les règles ont été suivies.

Et comme ils sont écrits à l'avance, lorsque vous écrivez votre code, vous pouvez suivre les règles sans avoir à expliquer votre raisonnement ni à avoir des arguments plus tard.

Si vous n'aimez pas les règles, organisez une réunion pour les modifier.

Je ne voudrais pas enduire vos commentaires, car ils peuvent être considérés comme condescendants.

À mon avis, la meilleure pratique consiste à toujours se concentrer sur le code et jamais sur l'auteur.

C'est une revue de code , pas une revue de développeur , donc:

• "Cette boucle while peut ne jamais finir", pas "Votre boucle ne peut jamais finir"
• "Un scénario de test est nécessaire pour le scénario X", pas "Vous n'avez pas écrit le test pour couvrir ce scénario"

Il est également très important d’appliquer la même règle lorsque vous parlez de l’examen avec d’autres:

• "Anne, que penses-tu de ce code?", Pas "Anne, que penses-tu du code de Jon?"

La révision du code n’est pas le moment de procéder à une évaluation de la performance - elle devrait être effectuée séparément.

Je suis surpris que cela n'ait été mentionné dans aucune réponse jusqu'à présent, et peut-être que mon expérience avec les revues de code est inhabituelle, mais:

Pourquoi passez-vous en revue l'intégralité de la demande de fusion dans un seul message?

Mon expérience avec les critiques de code est via GitLab; J'ai toujours imaginé que d'autres outils de révision de code fonctionneraient de la même manière.

Lorsque je passe en revue un code, je commente des lignes spécifiques du diff. Il est extrêmement improbable que cela reçoive une critique personnelle, car il s’agit d’un commentaire sur le code - et est en fait affiché comme commentaire sur le code, affiché directement sous le code sur lequel il est commenté.

Je peux également commenter la demande de fusion dans son ensemble et je le fais souvent. Mais des points spécifiques peuvent être signalés sur des lignes spécifiques du diff. (En outre, lorsqu'un nouveau commit est ajouté, de sorte que le diff change, les commentaires du "diff obsolète" sont masqués par défaut.)

Avec ce flux de travail, les révisions de code deviennent beaucoup plus modulaires et moins cohérentes. Une ligne d'une révision de code peut simplement dire,

Belle approche, envelopper cela dans une fonction dédiée!

Ou peut-être dire,

Ce nom d'objet ne correspond pas vraiment à l'objectif de l'objet; peut-être pourrions-nous utiliser un nom comme «XYZ» à la place?

Ou s'il y a des problèmes majeurs avec une section, je pourrais écrire:

Je vois que ce code fonctionne (et je vois pourquoi cela fonctionne), mais il nécessite une étude ciblée pour le comprendre. Pourriez-vous s'il vous plaît le reformuler dans des fonctions séparées afin qu'il soit plus facile à maintenir à l'avenir?

(Exemple: la fonction ABC fait trois choses ici: éblouir les foo, interdire les boz et froncer les zorf. Celles-ci pourraient toutes être des fonctions séparées.)

Après avoir rédigé tout ce qui précède, je peux formuler un commentaire de synthèse sur l’ensemble de la demande de fusion, à savoir:

Ceci est une excellente nouvelle fonctionnalité et il sera vraiment utile une fois fusionné. Pouvez-vous s'il vous plaît nettoyer la dénomination de la fonction et gérer la refactorisation mentionnée dans les commentaires individuels que j'ai faits, puis laissez-moi savoir pour l'examiner à nouveau? :)

Même si la demande de fusion concerne un petit-déjeuner complet pour un chien, les commentaires individuels peuvent être simples. Il y en aura juste plus. Ensuite, le commentaire récapitulatif pourrait dire:

Je suis désolé, mais ce code n'est pas vraiment à la hauteur. Un grand nombre de cas critiques (décrits dans des commentaires individuels) seront traités incorrectement et donneront une mauvaise expérience utilisateur, voire la corruption des données dans un cas. (Voir le commentaire sur le contrat 438a95fb734.) Même dans certains cas d'utilisation normaux, les performances de l'application sont extrêmement mauvaises (détails signalés dans les commentaires individuels sur le diff pour somefile.c).

Pour être prête pour la production, cette fonctionnalité nécessitera une réécriture complète en accordant plus d’attention à ce que les hypothèses sont sûres à faire à chaque point du flux. (Astuce: aucune hypothèse n'est sûre sauf si elle a été vérifiée.)

Je ferme la demande de fusion en attendant une réécriture complète.

Résumé: Examinez les aspects techniques du code en tant que commentaires sur des lignes de code individuelles. Résumez ensuite ces commentaires dans un commentaire général sur la demande de fusion. Ne restez pas personnel, expliquez simplement les faits et votre opinion sur le code , pas sur le codeur. Et basez votre opinion sur des faits, une observation précise et une compréhension.

Je suis vraiment surpris que personne ne l'ait relevé, mais il y a quelque chose qui cloche dans l'examen de l'échantillon posté.

Il n'y a tout simplement aucune raison pour que vous adressiez directement à Joe. Que Joe répare ses échecs ne vous regarde pas. C'est quelqu'un qui vous regarde. Votre souci est la qualité du code. Ainsi, au lieu d’écrire des demandes / ordres / demandes (qui, si j’étais Joe, je pourrais tout simplement refuser parce que vous n’êtes pas légitimes pour cela), restez à votre poste et écrivez une simple liste de tâches anonyme.

Essayer d'être juste en donnant les bons et les mauvais points n'est pas seulement impossible, mais complètement hors de votre rôle.

Voici un exemple de reformulation avec le contenu de votre avis:

• Avant d'extraire les modifications dans la classe Library \ ACME \ ExtractOrderMail, nous devons résoudre quelques problèmes.
• Sauf si quelque chose me manque, "TempFilesToDelete" ne devrait pas être statique.
• À l'avenir, nous pourrions appeler la fonction plus d'une fois par exécution, c'est pourquoi nous avons besoin de (ce qu'il faut faire ici).
• J'ai besoin de comprendre pourquoi "GetErrorMailBody" prend une exception en tant que paramètre. (et, je suis limite ici, parce que maintenant, vous devriez déjà avoir une conclusion )
• SaveAndSend doit être renommé pour mieux s'adapter à son comportement, comme par exemple "SendErrorMail"
• Le code commenté doit être supprimé pour des raisons de lisibilité. Nous utilisons subversion pour d'éventuelles annulations.

Si vous formulez l’examen ainsi, quel que soit le sens de la haine du lecteur, tout ce qu’il peut voir ici, c’est des notes sur les améliorations à apporter ultérieurement. Qui ? Quand ? Tout le monde s'en fout. Quoi ? Pourquoi ? QUE vous devriez dire.

Vous allez maintenant aborder la raison pour laquelle les critiques de code augmentent la tension en éliminant le facteur humain de l'équation.

Le but de la révision du code est de détecter les problèmes S'il y a un bogue, la meilleure chose à faire est d'écrire un scénario de test qui échoue.

Votre équipe (responsable) devrait vous informer que la production de bugs fait partie du jeu, mais les trouver et les corriger sauvera le travail de tout le monde .

Il pourrait être utile d’organiser des réunions régulières (en équipe ou en couple) et de passer en revue quelques problèmes. Le programmeur d'origine n'a pas introduit les problèmes intentionnellement, et parfois il pourrait penser que c'était assez bon (et même parfois). Mais avoir cette seconde paire d'yeux donne une vision complètement nouvelle, et il pourrait apprendre beaucoup en regardant les problèmes.

C'est vraiment une chose culturelle, et cela nécessite beaucoup de confiance et de communication. Et le temps de travailler avec les résultats.

Je pense que la chose positive à faire serait d’obtenir un consensus sur les normes de codage avant de procéder à des examens. D'autres ont tendance à acheter plus pour quelque chose quand ils ont leur mot à dire.

Pour quelque chose comme les conventions de nommage, demandez aux autres si cela est important. La plupart des développeurs seront d’accord, en particulier parmi leurs pairs. Qui veut être la personne qui ne veut pas être d'accord avec quelque chose d'aussi répandu dans le monde de la programmation? Lorsque vous démarrez le processus en sélectionnant le code d'une personne et en lui signalant sa faille, celui-ci devient très défensif. Lorsque les normes seront établies, il y aura désaccord sur l'interprétation ou sur ce qui est considéré comme "assez bon", mais vous êtes mieux loti que vous ne l'êtes maintenant.

Une autre partie de ce processus consiste à déterminer comment les révisions de code vont traiter les objections. Cela ne peut pas devenir un débat sans fin. À un moment donné, quelqu'un doit prendre la décision. Peut-être qu’il pourrait y avoir un tiers qui serait le juge ou le critique obtiendrait tout le pouvoir. Le but doit être atteint.

Le dernier élément est de faire savoir à tout le monde que les revues de code ne sont pas votre idée, elles vont rester, donc tout le monde devrait faire un effort pour en tirer le meilleur parti. Si tout le monde se sent impuissant, peut-être qu'il peut être autorisé à revoir votre code?

Espérons que certains résultats mesurables pour la direction seront de limiter les bugs, les plaintes des clients, les retards, etc. Sinon, quelqu'un entendra le mot à la mode "révision du code" et se dit que s'il l'ajoute à votre processus, des miracles se produiront sans trop de temps et d'énergie. et l'effort mis dans cela.

Cela peut être dur, mais ne vous inquiétez pas de donner de bons commentaires s'il n'y a rien de bon à mesurer.

Toutefois, à l'avenir, lorsque vos développeurs commenceront à améliorer leur code, vous voudrez bien leur faire part de leurs commentaires. Vous voudrez souligner les améliorations apportées au code, ainsi que les avantages pour l’ensemble de l’équipe. Lorsque vous constaterez une amélioration, ils le seront également et les choses commenceront lentement à se concrétiser.

Autre chose; il peut y avoir un air défensif parce qu'ils ont l'impression de ne pas avoir leur mot à dire. Pourquoi ne pas les laisser revoir le code de chacun? Posez-leur des questions spécifiques et essayez de les impliquer. Cela ne devrait pas être vous contre eux; cela devrait être un effort d'équipe.

1. Que changeriez-vous de ce code si vous en aviez le temps?
2. Comment amélioreriez-vous cette partie de la base de code?

Demandez-le maintenant et demandez-le dans six mois. Il y a une expérience d'apprentissage ici.

Le point principal - ne faites pas l'éloge du code là où ce n'est pas justifié, mais reconnaissez les efforts et reconnaissez clairement les améliorations. Essayez de faire des critiques un exercice en équipe au lieu d’un exercice contradictoire.

Qualité sans tension

Vous avez demandé comment vous pouvez trouver des choses positives à dire sur le code, mais votre vraie question est de savoir comment vous pouvez éviter les «tensions au sein de [votre] équipe» tout en abordant «les problèmes de qualité graves».

L'ancien truc consistant à prendre en sandwich «les mauvaises nouvelles dans les bonnes nouvelles» peut se retourner contre nous. Les développeurs sont susceptibles de le voir pour ce qu'il est: un dispositif.

Vos organisations des problèmes de haut en bas

Vos chefs vous ont confié la tâche d’assurer la qualité. Vous avez proposé une liste de critères pour la qualité du code. Désormais, vous souhaitez que des idées de renforcement positif soient fournies à votre équipe.

Pourquoi nous demander ce que vous devez faire pour rendre votre équipe heureuse? Avez-vous envisagé de demander à votre équipe?

On dirait que vous faites de votre mieux pour être gentil. Le problème n’est pas de savoir comment vous transmettez votre message. Le problème est que la communication a été à sens unique.

Construire une culture de qualité

Une culture de qualité doit être nourrie pour se développer de bas en haut.

Dites à votre patron que vous craignez que la qualité ne s'améliore pas assez vite et que vous souhaitiez appliquer les conseils de The Harvard Business Review .

Rencontrez votre équipe. Modélisez les comportements que vous souhaitez voir dans votre équipe: humilité, respect et engagement à s'améliorer.

Dites quelque chose comme: «Comme vous le savez, [collègue] et moi avons été chargés de garantir la qualité du code, afin de prévenir le type de problèmes de production que nous avons connus récemment. Personnellement, je ne pense pas avoir résolu ce problème. Je pense que ma plus grande erreur ne vous a pas tous impliqué plus au début. [collègue] et moi sommes toujours responsables devant la direction pour la qualité du code, mais nous allons tous ensemble dans le sens de la qualité. ”

Obtenez des idées de l'équipe sur la qualité du code. (Un tableau blanc pourrait vous aider.) Assurez-vous que vos exigences sont bien définies. Proposez vos idées de manière respectueuse et posez des questions, le cas échéant. Soyez prêt à être surpris de ce que votre équipe juge important. Soyez flexible, sans compromettre les besoins de votre entreprise.

(Si vous avez quelque chose de vieux code qui vous embarrasse, vous pourriez le jeter, encourageant tout le monde à être franc. À la fin, faites savoir aux gens que vous l'avez écrit. Modélisez la réaction mûre que vous espérez lorsque d'autres reçoivent des critiques de construction. )

Commentaires collaboratifs sur les codes

Je n'ai pas travaillé dans un endroit comme vous le décrivez, où quelques programmeurs chevronnés examinent tout le code et apportent des corrections. Pas étonnant que les gens réagissent comme si vous étiez un enseignant et faites des marques rouges sur leurs papiers.

Si vous pouvez vendre l'idée à la gestion, commencez à examiner les codes de vos pairs . Cela se fait mieux en petits groupes, y compris vous-même ou un autre développeur responsable. Assurez-vous que tout le monde est traité avec respect.

Un objectif important du code de révision par les pairs est de s'assurer que le code peut être compris par tous les membres de l'équipe. Pensez à votre exemple de noms de fonctions peu clairs: il est plus facile d’accepter, pour un développeur plus expérimenté, que le nom de la fonction prête à confusion, qu’une autre «correction» de la part du développeur principal.

La qualité est un voyage

Une autre chose à faire est de supprimer toute idée selon laquelle une révision de code est une sorte de réussite ou d’échec. Tout le monde devrait s'attendre à apporter des modifications après une révision du code. (Et votre travail est de veiller à ce qu'ils se produisent.)

Mais si ça ne marche pas ...

Supposons que vous faites des progrès en établissant une culture de la qualité. Vous ne pouvez toujours pas obtenir tout le monde à bord. Si quelqu'un ne participe toujours pas au programme de qualité, le problème est maintenant qu'ils ne font pas partie de l'équipe, plutôt qu'un problème entre vous deux. S'ils doivent être renvoyés de l'équipe, le reste de l'équipe comprendra mieux les raisons.

Désolé pour une autre longue réponse, mais je ne pense pas que les autres ont pleinement abordé l’aspect humain de cette question.

parfois même simplement demander pourquoi quelque chose a été mis en œuvre de manière spécifique. Quand je pense que c'est mauvais, je souligne que je l'aurais développé d'une autre manière.

Le problème avec "Pourquoi l'avez-vous implémenté de cette façon?" est-ce que vous mettez le développeur immédiatement sur la défensive. La question elle-même peut impliquer toutes sortes de choses en fonction du contexte: êtes-vous trop stupide pour penser à une meilleure solution? Est-ce le mieux que vous puissiez faire? Essayez-vous de ruiner ce projet? Vous souciez-vous même de la qualité de votre travail? etc. Demander à "pourquoi" le code a été développé d'une certaine manière ne sera que conflictuel, et cela détournera toute intention pédagogique que votre commentaire aurait pu avoir.

De même, "J'aurais fait cela différemment ..." est également conflictuel, car le développeur pensait immédiatement: " C'est ce que j'ai fait de cette façon ... Vous avez un problème? " Et encore, c'est plus conflictuel que cela doit être et détourne la discussion de l'amélioration du code.

Exemple:

Au lieu de demander : « Pourquoi avez - vous choisi de ne pas utiliser la variable constante de cette valeur? », Indiquer simplement « Cette valeur codée en dur doit être remplacé par la constante XYZdans le dossier Constants.h» Poser la question suggère que le développeur a choisi activement pas utiliser la constante déjà définie, mais il est tout à fait possible qu'ils ne savaient même pas qu'elle existait. Avec une base de code assez importante, il y aura beaucoup de choses que chaque développeur ne sait pas. Il s’agit simplement d’une bonne occasion d’apprentissage pour le développeur qui souhaite se familiariser avec les constantes du projet.

Il y a une bonne marge de manœuvre pour passer en revue les critiques de code: vous n'avez pas besoin d'enrober tout ce que vous dites, vous n'avez pas besoin de combiner une mauvaise nouvelle avec une bonne nouvelle et vous n'avez pas besoin d'atténuer le coup. C’est peut-être la culture dans laquelle je me trouve, mais mes collègues et moi-même ne nous complimentons jamais dans la révision du code - les parties du code que nous développons qui sont exemptes de défauts sont assez compliment. Ce que vous devez faire, c'est identifier le défaut que vous voyez, et peut-être en donner la raison (le pourquoi est moins utile lorsqu'il est évident / simple).

Bien:

• "Le nom de cette variable doit être modifié pour correspondre à notre norme de codage."

• "Le 'b' dans ce nom de fonction doit être mis en majuscule pour être PascalCase."

• "Le code de cette fonction n'est pas mis en retrait correctement."

• "Ce code est un duplicata du code présent dans ABC::XYZ(), et devrait utiliser cette fonction à la place."

• "Vous devez utiliser try-with-resources pour garantir la fermeture des éléments dans cette fonction, même en cas d'erreur." [J'ai seulement ajouté un lien ici pour que les utilisateurs non-java sachent ce que signifie "essayer avec les ressources"]

• "Cette fonction doit être revue afin de répondre à nos normes de complexité" n-path "."

Mauvais:

• "Je pense que vous pourriez améliorer ce code en modifiant le nom de la variable afin qu'il corresponde à notre norme"

• " Peut -être serait - il préférable d'utiliser try-with-resource pour fermer correctement les éléments de cette fonction"

• "Il serait peut- être souhaitable d'examiner de nouveau toutes les conditions de cette fonction. La complexité de N-path dépasse la complexité maximale autorisée par notre standard."

• "Pourquoi les retraits ici sont-ils 2 espaces au lieu de 4 pour notre standard?"

• "Pourquoi avez-vous écrit une fonction qui casse notre standard de complexité de n-chemin?"

Dans les mauvaises déclarations, les parties en italiques sont des éléments que les gens utilisent couramment pour atténuer les conséquences, mais tout ce que fait une révision de code consiste à vous faire paraître incertain. Si vous, l'examinateur, n'êtes pas certain de la manière d'améliorer le code, pourquoi quelqu'un devrait-il vous écouter?

Les «bonnes» déclarations sont franches, mais elles n'accusent pas le développeur, elles n'attaquent pas le développeur, elles ne sont pas conflictuelles et elles ne se demandent pas pourquoi le défaut existe. Ça existe; voici la solution. Ils ne sont certainement pas aussi conflictuels que la dernière question de savoir pourquoi.

Le problème que vous voyez est le suivant: les développeurs sont mécontents du fait que leur code soit critiqué. Mais ce n'est pas le problème. Le problème est que leur code n'est pas bon (en supposant évidemment que vos critiques de code sont bonnes).

Si les développeurs n'aiment pas que leur code suscite des critiques, la solution est simple: rédigez un meilleur code. Vous dites que vous avez eu de graves problèmes de qualité du code; cela signifie qu'un meilleur code est nécessaire.

"Pourquoi la méthode devrait-elle avoir un nom raisonnable, je sais ce qu'elle fait?" est quelque chose que je trouve particulièrement dérangeant. Il sait ce que ça fait, ou du moins il le dit, mais moi non. Toute méthode ne doit pas simplement avoir un nom logique, elle doit également avoir un nom qui permette au lecteur du code de comprendre immédiatement ce qu’il fait. Vous voudrez peut-être consulter le site Web d’Apple pour visionner une vidéo WWDC sur les modifications apportées de Swift 2 à Swift 3 - un grand nombre de modifications apportées simplement pour rendre tout ce qui est plus lisible. Ce type de vidéo pourrait peut-être convaincre vos développeurs que ceux qui sont bien plus intelligents trouvent les noms de méthodes intuitives très importants.

Un autre élément troublant a été votre collègue, qui a déclaré: "Elle m’a dit elle-même qu’après le signalement d’un rapport de bug par un client, elle était au courant du bogue, mais craignait que le développeur ne soit en colère contre elle pour le lui avoir signalé". Il est possible qu'il y ait un malentendu, mais si un développeur se met en colère si vous lui signalez un bogue, c'est mauvais.

Je serais respectueusement en désaccord avec la méthode de révision du code que vous avez décrite. Deux membres du personnel se rendant dans une "pièce fermée" et émettant des critiques institutionnalisent la notion même accusatoire que les bonnes revues de code devraient éviter .

Faire de la révision du code une expérience positive dans la mesure du possible est essentiel à sa réussite. La première étape consiste à supprimer la notion contradictoire d’examen. Faites-en un événement de groupe hebdomadaire ou bihebdomadaire ; assurez-vous que tout le monde sait qu'ils sont invités à participer. Commandez des pizzas ou des sandwichs ou autre chose. N'appelez même pas cela une «révision de code» si cela suscite des connotations négatives. Trouvez quelque chose à célébrer, à encourager, à partager - même s'il ne s'agit que de progresser dans le sprint ou l'itération en cours. À tour de rôle, attribuez le leadership à l’examen.

Faites du processus un effort pour servir le produit et les gens. Si elles sont effectuées de manière constructive, positive, dans laquelle les bonnes techniques ou modèles sont partagés et encouragés tout comme les pauvres sont découragés - tout le monde en profite. Tout le monde est coaché ​​en public. Si vous avez un programmeur à problèmes qui ne «comprend pas», vous devez les aborder séparément et en privé, jamais devant un groupe plus large. C'est séparé de l'examen du code.

Si vous avez du mal à trouver quelque chose de «bon» à aborder, cela me pose une question: si des progrès sont réalisés dans le projet et que les gens travaillent, ces progrès en eux-mêmes sont à célébrer. Si vous ne trouvez vraiment rien de bon, cela signifie que tout ce qui a été fait depuis le dernier examen n’est ni mauvais ni meilleur que neutre . Est-ce vraiment le cas?

En ce qui concerne les détails, des normes communes sont essentielles. Donnez à chacun un intérêt dans ce qui se fait. Et permettez l'intégration de nouvelles techniques plus performantes dans votre base de code. L'échec à faire cela garantit que les vieilles habitudes ne sont jamais excisées sous le prétexte de "nous l'avons toujours fait de cette façon".

Le problème, c’est que les critiques de code sont mal vues parce qu’elles sont mal mises en œuvre, utilisées comme des marteaux pour rabaisser les programmeurs moins expérimentés ou moins qualifiés, et cela n’est utile à personne. Les gestionnaires qui les utilisent de cette manière ne seront probablement pas très efficaces. De bonnes critiques de code dans lesquelles tout le monde est un participant sert tout le monde - le produit, les employés et l'entreprise.

Toutes mes excuses pour la longueur du message, mais après avoir été dans un groupe où la révision de code a été en grande partie ignorée, cela a laissé un héritage de code intangible et un nombre restreint de développeurs capables, même s’ils étaient autorisés à modifier un code vieux de plusieurs années. C’était un chemin que je choisirais de ne plus parcourir.

Le paradoxe d'un bon code est qu'il ne se démarque pas du tout et semble très simple et facile à écrire. J'aime beaucoup l'exemple du joueur de billard tiré de cette réponse . Dans les revues de code, il est très facile de passer inaperçu, à moins que cela ne soit un refactor de mauvais code.

Je me fais un devoir de le chercher. Si je vérifie le code et que je suis passé par un fichier si facile à lire qu'il semble trompeur de facilité à écrire, je viens d'ajouter un rapide "J'aime la façon dont les méthodes ici sont courtes et propres" ou ce qui convient .

En outre, vous devriez donner l'exemple. Vous devez insister pour que votre code soit également révisé et vous devez indiquer comment vous voulez que votre équipe se comporte lors de la réception des corrections. Plus important encore, vous devriez sincèrement remercier les gens pour leurs commentaires. Cela fait plus de différence que n'importe quelle rétroaction unilatérale que vous pouvez donner.

Le vrai problème, c’est que c’est seulement deux personnes qui effectuent toutes les révisions de code, dont vous êtes les seuls à le prendre au sérieux, ce qui vous a conduit à une situation malheureuse avec beaucoup de responsabilité et beaucoup la pression des autres membres de l'équipe.

Un meilleur moyen serait de répartir la responsabilité de la révision du code sur l’équipe et de faire participer tout le monde, par exemple en laissant les développeurs choisir qui réviser leur code. C’est quelque chose que votre outil de révision de code devrait prendre en charge.

Je l'ai vu de mes propres yeux et je tiens à vous dissuader de répondre aux questions de l' intelligence émotionnelle - elles peuvent tuer des équipes entières. À moins que vous ne souhaitiez recruter, former et normaliser de nouveaux développeurs chaque année, il est essentiel de créer une relation positive avec vos pairs. Après tout, l'objectif de ces examens n'est-il pas d'améliorer la qualité du code et de favoriser une culture où la qualité du code est supérieure? Je dirais que votre travail consiste en effet à fournir un renforcement positif comme moyen d’intégrer ce système de révision de code dans la culture de l’équipe.

Quoi qu'il en soit, il semble que vous deviez revoir votre système de révision de code. À l'heure actuelle, tout semble dans votre processus d'examen est, ou peut être interprété, subjectif plutôt qu'objectif. Il est facile de se sentir blessé quand on a l'impression que quelqu'un sépare votre code parce qu'il ne l'aime pas, plutôt que d'avoir une raison de pouvoir citer quand quelque chose ne correspond pas aux directives. De cette façon, il est facile de suivre et de «célébrer» les améliorations positives de la qualité du code (en ce qui concerne votre système de révision) de la manière qui convient le mieux à la culture de votre entreprise.

Les responsables techniques doivent se réunir en dehors d'une session de révision et établir une liste de contrôle des directives de codage / révision du code. Cette dernière doit être respectée et référencée lors de la révision. Cela devrait être un document évolutif qui peut être mis à jour et affiné à mesure que le processus avance. Ces réunions de responsables techniques devraient également avoir lieu lorsque la discussion «Comment nous avons toujours fait les choses» par rapport à «De nouvelles façons améliorées de faire les choses» devrait avoir lieu, sans que le développeur soumis à l'examen ait le sentiment que le désaccord résulte de son code. Une fois que la ligne directrice initiale a été plus ou moins lissée, un autre moyen de renforcer positivement les développeurs est de demander leur feedback et de prendre des mesures concrètes à cet égard. et faire évoluer le processus en tant qu'équipe, cela fait des merveilles de les mettre au courant pour commencer à réviser le code des cartes embarquées afin que vous ne soyez pas obligé de faire des révisions jusqu'à votre retraite.

Locaux...

Les programmeurs résolvent les problèmes. Nous sommes conditionnés pour commencer le débogage lorsque nous rencontrons un problème ou une erreur.

Le code est un support au format hiérarchique. Le passage à une narration sous forme de paragraphe pour les commentaires introduit une déconnexion qui doit être traduite. Inévitablement, quelque chose est perdu ou mal compris. Inévitablement, le critique ne parle pas dans la langue du programmeur.

Les messages d'erreur générés par ordinateur sont rarement remis en question et ne sont jamais considérés comme un affront personnel.

Les éléments de révision de code sont des messages d'erreur générés par l'homme. Ils sont conçus pour détecter les cas extrêmes qui manquent aux programmeurs et aux outils automatisés, ainsi que les effets secondaires inévitables sur d'autres programmes et données.

Conclusions ...

Ainsi, plus les éléments de révision de code peuvent être intégrés à des outils automatisés, mieux ils seront reçus.

La mise en garde est que, pour rester incontestés, ces outils doivent être continuellement mis à jour, généralement tous les jours ou toutes les semaines, pour être en conformité avec tout changement apporté aux procédures, normes et pratiques. Lorsqu'un responsable ou une équipe de développeurs décide d'apporter un changement, les outils doivent être modifiés pour le mettre en œuvre. Une grande partie du travail du réviseur de code consiste alors à maintenir les règles dans les outils.

Exemple de commentaires de révision de code ...

Une réécriture de l'exemple donné incorporant ces techniques:

• Matière:

  • Bibliothèque \ ACME \ ExtractOrderMail Class.

• Problème principal ...

  • TempFilesToDelete est statique
    • Les appels suivants à GetMails émettent une exception car des fichiers y sont ajoutés mais ne sont jamais supprimés après la suppression. Bien qu’un seul appel maintenant, les performances pourraient être améliorées à l’avenir avec un peu de parallélisme.
    • TempFilesToDelete en tant que variable d'instance autoriserait l'utilisation de plusieurs objets en parallèle.
• Problèmes secondaires ...
  • GetErrorMailBody a un paramètre d'exception
    • Puisqu'il ne lève pas une exception elle-même, et le transmet simplement à ToString, est-ce nécessaire?
  • SaveAndSend name
    • Le courrier électronique peut ou ne peut pas être utilisé pour signaler ce problème à l'avenir. Ce code contient la logique générale permettant de stocker une copie persistante et de signaler les erreurs éventuelles. Un nom plus générique permettrait ces modifications anticipées sans modification des méthodes dépendantes. StoreAndReport est une possibilité.
  • Commentant l'ancien code
    • Le fait de laisser une ligne commentée et marquée OBSOLETE peut être très utile pour le débogage, mais un "mur de commentaires" peut également masquer des erreurs dans du code adjacent. Nous l'avons toujours dans Subversion. Peut-être juste un commentaire référençant exactement où dans Subversion?

Si vous n'avez rien de bien à dire sur le code existant (ce qui semble compréhensible), essayez d'impliquer le développeur dans l'amélioration.

Dans un correctif pour une modification fonctionnelle ou un correctif, il n'est pas utile de regrouper d'autres modifications (renommer, refactoriser, etc.) dans le même correctif. Il convient que le changement qui corrige le bogue ou ajoute la fonctionnalité, rien d’autre.

Lorsque le changement de nom, le refactoring et d’autres modifications sont indiqués, effectuez-les dans un patch séparé, avant ou après.

• C'est assez moche, mais j'imagine que cela correspond à tous nos autres codes désagréables. Nettoyons cela plus tard (dans un patch avec, idéalement, aucun changement fonctionnel).

  Cela vous aidera également si vous participez à la refactorisation et laissez- les réviser votre code. Cela vous donne une chance de dire pourquoi vous pensez que quelque chose est bon, plutôt que de mal, et aussi de montrer un exemple de bien prendre la critique constructive.

Si vous devez ajouter des tests unitaires à une nouvelle fonctionnalité, très bien, ils vont dans le patch principal. Mais si vous corrigez un bogue, les tests doivent figurer dans un correctif précédent et ne pas réussir , vous pouvez donc démontrer que le correctif corrige réellement le bogue.

Idéalement, le plan (pour savoir comment vous pouvez tester un correctif, ou ce qu'il faut refactoriser et quand) doit être discuté avant que le travail ne soit terminé, afin qu'ils n'aient pas consacré beaucoup d'effort à quelque chose avant de découvrir que vous avez un problème avec.

Si vous pensez que le code ne prend pas en charge les entrées que vous croyez devoir le faire, cela peut également constituer une incohérence dans ce que le développeur considère comme une entrée raisonnable. Acceptez cela à l'avance aussi, si possible. Au moins, discutez de ce à quoi il devrait être capable de faire face et de la méchanceté avec laquelle on peut le laisser échouer.

Je pense que c'est une erreur de supposer qu'il existe une solution technique ou simple à: "mes collègues sont bouleversés lorsque je juge leur travail à la lumière de mes critères et ont un certain pouvoir pour le faire respecter".

Rappelez-vous que les critiques de code ne sont pas simplement une discussion de ce qui est bon ou mauvais. Ils sont implicitement un "qui bâillons nous utilisons", un "qui prend la décision" et donc le plus insidieusement, un rang.

Si vous n'abordez pas ces points, il sera difficile de réviser le code d'une manière qui ne présente pas les problèmes que vous rencontrez. Il y a quelques bons conseils ici sur la façon de le faire, mais vous vous êtes fixé une tâche difficile.

Si vous avez raison, sans aucune marge de manoeuvre, le signaler est une attaque: quelqu'un s'est trompé. Si non: votre autre MBA qui ne l'obtient pas. De toute façon, vous allez être le méchant.

Toutefois, si l'examen ce que le cherche et pourquoi , est claire et convenue, vous avez une bonne chance de ne pas être le méchant. Vous n'êtes pas le gardien, mais un correcteur d'épreuves. Obtenir cet accord est un problème difficile à résoudre [1]. J'aimerais vous donner un conseil, mais je ne l'ai pas encore maîtrisé moi-même ...

[1] Ce n'est pas résolu simplement parce que les gens ont dit "ok", ou ont cessé de se battre à ce sujet. Personne ne veut être du genre à dire que X n'est pas pratique pour notre {intelligence, notre expérience, notre force de frappe, nos échéances, etc.} mais cela ne signifie pas qu'il s'agisse en fait d'un cas spécifique de faire X ...

Les revues de code doivent être mutuelles. Tout le monde a critiqué tout le monde. Que la devise soit: "Ne vous fâchez pas, faites le même". Tout le monde fait des erreurs et une fois que les gens auront expliqué comment corriger leur code, ils comprendront que c'est juste la procédure normale et non une attaque contre eux.

Voir le code des autres est éducatif. Comprendre le code au point de pouvoir identifier son point faible et devoir expliquer que cette faiblesse peut vous apprendre beaucoup!

Il y a peu de place pour des éloges dans une révision de code, car l’essentiel est de trouver des défauts. Cependant, vous pouvez faire des éloges sur les correctifs . La rapidité et la propreté peuvent être louées.