Comment puis-je suggérer avec tact des améliorations au code mal conçu des autres lors de la révision?

Je suis un fervent partisan du code propre et de son savoir-faire, même si je suis actuellement dans un poste où cela n'est pas considéré comme une priorité absolue. Je me trouve parfois dans une situation où le code d'un collègue est semé d'embûches dans une conception en désordre et ne tient guère à la maintenance future, bien qu'il soit fonctionnel et ne contienne que peu, voire aucun bogue.

Comment faites-vous pour suggérer des améliorations dans la révision du code lorsque vous pensez qu'il y a tellement de choses qui doivent être changées et qu'il y a une échéance à venir? Gardez à l'esprit que suggérer que les améliorations soient apportées après la date limite peut signifier qu'elles seront totalement hiérarchisées au fur et à mesure que de nouvelles fonctionnalités et corrections de bugs entrent en jeu.

1. Vérifiez votre motivation. Si vous pensez que le code devrait être modifié, vous devriez pouvoir expliquer pourquoi vous pensez qu’il devrait être modifié. Et cette raison devrait être plus concrète que "je l'aurais fait différemment" ou "c'est moche". Si vous ne parvenez pas à souligner un avantage découlant du changement que vous proposez, il ne sert à rien de perdre du temps (autrement dit de l'argent) à le changer.

2. Chaque ligne de code du projet est une ligne à gérer. Le code devrait être aussi long que nécessaire pour faire le travail et être facilement compris, et plus. Si vous pouvez raccourcir le code sans sacrifier la clarté, tant mieux. Si vous pouvez le faire en augmentant la clarté, c'est beaucoup mieux.

3. Code est comme concret: il est plus difficile de changer après avoir été assis pendant un moment. Suggérez vos modifications dès que possible, de manière à minimiser les coûts et les risques liés aux modifications.

4. Chaque changement coûte de l'argent. Réécrire du code qui fonctionne et qui n’aura probablement pas besoin d’être modifié pourrait être un effort inutile. Concentrez votre attention sur les sections les plus susceptibles de changer ou qui sont les plus importantes pour le projet.

5. La forme suit la fonction et parfois l'inverse. Si le code est en désordre, il est fort probable qu'il contienne également des bogues. Recherchez ces bogues et critiquez la fonctionnalité défectueuse plutôt que l’attrait esthétique du code. Suggérer des améliorations qui rendent le travail de code mieux et rendre plus faciles à vérifier le bon fonctionnement du code.

6. Différencier conception et mise en œuvre. Une classe importante avec une interface de merde peut se propager à travers un projet comme le cancer. Cela ne réduira pas seulement la qualité du reste du projet, mais augmentera également la difficulté de réparer les dégâts. D'autre part, une classe avec une interface bien conçue, mais une implémentation moche ne devrait pas être un gros problème. Vous pouvez toujours réimplémenter la classe pour améliorer les performances ou la fiabilité. Ou, si cela fonctionne correctement et est assez rapide, vous pouvez le laisser seul et vous sentir en sécurité en sachant que sa crue est bien encapsulée.

Pour résumer tous les points ci-dessus: Assurez-vous que les modifications proposées apportent une valeur ajoutée.

Il existe un bon compromis pour ajouter de la valeur par le biais de la refactorisation. Les changements doivent accomplir trois choses:

• améliorer le code susceptible de changer
• augmenter la clarté
• coût moindre effort

Considérations:

1. Nous savons que le code propre coûte moins cher à écrire et à maintenir et qu'il est plus amusant de travailler dessus. Votre travail consiste à vendre cette idée aux membres de votre entreprise. Pensez comme un vendeur, pas comme un grognon arrogant (c'est-à-dire pas comme moi).
2. Vous ne pouvez pas gagner, vous ne pouvez que perdre moins.
3. Concentrez-vous sur l'ajout de valeur réelle - pas seulement sur la beauté. J'aime que mon code soit beau, mais je dois parfois accepter que les questions peu coûteuses importent davantage.
4. Un bon moyen de trouver le bon compromis est de suivre le principe du scoutisme - lorsque vous travaillez sur une zone de code, laissez-la toujours meilleure que celle que vous avez trouvée.
5. Une petite amélioration vaut mieux que pas d'amélioration.
6. Faites bon usage des outils automatisés. Par exemple, les outils qui nettoient juste un peu de mise en forme peut faire un monde de différence.
7. Vendez d'autres idées qui, accessoirement, améliorent la clarté du code. Par exemple, les tests unitaires encouragent la décomposition de méthodes volumineuses en méthodes plus petites.

Si le code fonctionne sans bugs graves et qu’une échéance majeure (comme dans les effets P & L ou PR en entreprise) est imminente, il est trop tard pour suggérer des améliorations nécessitant des modifications majeures. Même les améliorations de code peuvent créer des risques pour le déploiement de projets. Le temps pour les améliorations était plus tôt dans le projet, quand il y avait plus de temps pour investir dans la robustesse future de la base de code.

La révision du code a trois objectifs:

1. Vérifier les bugs

2. Vérifier pour voir où le code pourrait être amélioré

3. Outil pédagogique pour quiconque a écrit le code.

L'évaluation de la qualité de la conception / du code concerne bien sûr les points 2 et 3.

Aussi loin que n ° 2:

• Expliquez très clairement quels sont les avantages des modifications proposées par rapport aux coûts à réparer. Comme toute décision d’affaires, il devrait s’agir d’une analyse coûts / avantages.

  Par exemple, "l'approche X en matière de conception réduirait considérablement la probabilité que le bogue Y se produise lors d'une modification Z, et nous savons que ce code subit des modifications de type Z toutes les deux semaines. Le coût de la gestion de la panne de production due au bogue Y + recherche du bogue + réparer et libérer le correctif + coût d’opportunité de ne pas livrer la prochaine série d’explorations est $A, alors que le coût de nettoyage du code maintenant et le coût d’opportunité (par exemple, prix d’expédition tardive ou avec moins de fonctionnalités) sont $B. Demandez à votre chef d'équipe / manager - évaluer $Avs $Bet décider.

  • Cela aidera l'équipe intelligente à gérer cela efficacement. Par exemple, ils prendront une décision rationnelle en utilisant les informations complètes

  • Cela augmentera VOTRE statut (surtout si vous le dites bien) - par exemple, vous êtes quelqu'un de suffisamment intelligent pour voir les avantages d'un meilleur design, ET assez intelligent pour ne pas le réclamer religieusement sans peser de considérations commerciales.

  • ET, dans le cas probable où le bug Z se produirait, vous gagnerez encore plus en profitant de la suite de suggestions.

Aussi loin que n ° 3:

• TRÈS clairement définir "doit corriger" les bogues / problèmes de "Ceci est une meilleure pratique et doit vraiment être corrigé SI nous pouvons économiser des ressources - voir ci-jointe analyse des avantages / inconvénients" améliorations de la conception (attachez les éléments décrits au n ° 2 ci-dessus) vs " Je pense que ce sont des instructions générales qui pourraient vous aider à améliorer la robustesse de votre code afin que vous puissiez plus facilement gérer le code "modifications facultatives". S'il vous plaît noter les termes - il ne s'agit pas de "rendre votre code comme ce que je veux" - c'est "si vous faites cela, vous gagnez des avantages a, b, c". Le ton et l'approche comptent.

Choisissez vos batailles, si une date limite approche, ne faites rien. La prochaine fois que quelqu'un examinera ou mettra à jour le code et qu'il continuera à avoir des problèmes, invitez-le à penser qu'en tant qu'équipe, vous devez vous concentrer davantage sur la qualité du code lors de la révision du code afin d'éviter tout problème ultérieur.

Ils devraient voir la valeur avant de faire le travail supplémentaire.

Je commence toujours mon commentaire par "Je voudrais", ce qui signifie que le mien n’est que l’un des nombreux points de vue.

J'ai aussi toujours inclure une raison.

" Je voudrais extraire ce bloc dans une méthode pour des raisons de lisibilité."

Je commente tout; grand et petit. Parfois, je fais plus d'une centaine de commentaires sur un changement. Dans ce cas, je recommande également la programmation en binôme et me propose comme homme des ailes.

J'essaie d'établir un langage commun pour des raisons; lisibilité, DRY, SRP, etc.

J'ai également créé une présentation sur le code propre et le refactoring, expliquant pourquoi et montrant comment, que j'ai réservée à mes collègues. Je l’ai tenu trois fois jusqu’à présent, et un cabinet de conseil que nous utilisons me demande de le lui réserver à nouveau.

Mais certaines personnes ne veulent pas écouter quand même. Ensuite, il me reste à tirer le rang. Je suis le responsable de la conception. La qualité du code est ma responsabilité. Ce changement ne passera pas sous ma montre dans son état actuel.

Veuillez noter que je suis plus que disposé à faire marche arrière sur tout commentaire que je fais; pour des raisons techniques, délais, prototypes, etc.

Oh, et j'ai récemment proposé d'acheter le déjeuner au premier membre de mon équipe qui a soumis un changement non trivial sur lequel je n'avais aucun commentaire. (Hey, vous devez aussi vous amuser. :-)

Je me trouve parfois dans une situation où le code d'un collègue est semé d'embûches dans une conception en désordre et ne tient guère à la maintenance future, bien qu'il soit fonctionnel et ne contienne que peu, voire aucun bogue.

Ce code est fait. À un moment donné, les remaniements deviennent trop coûteux pour être justifiés. Si le code est déjà fonctionnel avec peu ou pas de bogues, ce sera une vente impossible. Suggérez quelques façons de nettoyer cela à l’avenir et de continuer. Si / quand le code tombe dans le futur, réévaluez alors la valeur d'une refonte. Cela peut ne jamais casser, ce qui serait génial. Quoi qu’il en soit, vous êtes sur le point de parier que cela ne va pas casser, car le coût sera le même maintenant ou plus tard: une longue et longue reconfiguration.

Ce que vous devez faire à l’avenir est d’avoir des itérations de développement plus strictes. Si vous aviez pu réviser ce code avant que tout le travail d'élimination des bugs ait été investi, il aurait été logique de suggérer une refonte. Vers la fin, il n’a jamais de sens de faire une refactorisation majeure à moins que le code ne soit écrit d’une manière fondamentalement intenable et que vous sachiez avec certitude que le code devra être modifié peu de temps après sa publication.

Si vous avez le choix entre les deux options (refactor ou pas de refactor), réfléchissez à ce qui ressemble à la vente la plus intelligente:

Salut patron, nous étions dans les temps et tout fonctionnait, mais nous allons maintenant reconstruire beaucoup de choses pour pouvoir ajouter la fonctionnalité X à l’avenir.

ou

Salut patron, nous sommes prêts à libérer. Si nous devions ajouter la fonctionnalité X, cela pourrait nous prendre quelques jours de plus.

Si vous dites l'une ou l'autre, votre patron dira probablement:

Qui a parlé de la fonctionnalité X?

En fin de compte, il est parfois judicieux de prendre une dette technique si vous n’avez pas été en mesure de corriger certaines imperfections alors qu’elle était peu coûteuse (premières itérations). La conception de code de qualité génère des rendements décroissants à mesure que vous vous rapprochez d'une fonctionnalité achevée et de la date limite.

[Cette réponse est un peu plus large que la question initiale, puisqu'il s'agit de la redirection pour de nombreuses autres questions sur les revues de code.]

Voici quelques principes que je trouve utiles:

Critiquez en privé, louangez publiquement. Informez quelqu'un d'un bug dans son code personnel. S'ils font quelque chose de génial ou assument une tâche dont personne ne voulait, félicitez-le lors d'une réunion de groupe ou envoyez-lui un e-mail envoyé à l'équipe.

Partagez vos propres erreurs. Je partage l’histoire de ma première revue de code (reçue) désastreuse avec des étudiants et des collègues plus jeunes. Je fais également savoir aux étudiants que j'ai attrapé leur virus si rapidement parce que je l'ai fait avant moi-même. Dans une révision de code, ceci pourrait donner l’affirmation suivante: "Je pense que vous avez mal interprété les variables d’index. Je le vérifie toujours à cause du temps où j’ai mal fait mes index et jeté un centre de données en panne." [Oui, c'est une histoire vraie.]

N'oubliez pas de faire des commentaires positifs. Un bref "sympa!" ou "astuce!" peut faire la journée d'un programmeur junior ou peu sûr.

Supposons que l'autre personne soit intelligente mais parfois négligente. Ne dites pas: "Comment voulez-vous que l'appelant obtienne la valeur de retour si vous ne la renvoyez pas réellement?" Dites: "On dirait que vous avez oublié la déclaration de retour." Rappelez-vous que vous avez écrit un code affreux à vos débuts. Comme quelqu'un l'a déjà dit: "Si vous n'avez pas honte de votre code d'il y a un an, vous n'apprenez pas."

Enregistrez le sarcasme / ridicule pour les amis pas sur votre lieu de travail. Si le code est terriblement mauvais, plaisantez ailleurs. (Je trouve cela pratique d’être marié à un collègue programmeur.) Par exemple, je ne partagerais pas les dessins suivants (ou celui-ci ) avec mes collègues.

Quand une cuillerée de sucre aide le médicament à baisser et que ce qui ne va pas peut être exprimé de manière succincte - il n'y a pas 20 choses qui ne vont pas bien - je vais vous présenter un formulaire qui suggère que je n'ai aucun enjeu, aucun ego investi dans ce que je veux. être entendu. Habituellement c'est quelque chose comme:

Je me demande s'il serait préférable de ...

ou

Cela a-t-il un sens de ...

Si les raisons sont assez évidentes, je ne les énonce pas. Cela donne aux autres une chance d'assumer une partie de la propriété intellectuelle de la suggestion, comme dans:

"Oui, c'est une bonne idée, parce que < votre raison évidente ici >."

Si l'amélioration est assez évidente, mais pas tant que cela me donne l'air idiot de ne pas y penser, et que la raison de le faire reflète une valeur partagée avec l'auditeur, alors parfois, je ne le suggère même pas, à la place:

Je me demande s'il y a un moyen de ... <déclaration de valeur partagée ici>

Ceci est uniquement pour traiter avec des personnes très sensibles - avec la plupart de mes pairs, je les laisse juste l'avoir!

Les revues de code ne visent pas toujours à apporter des améliorations.

Une revue vers la fin d'un projet, comme cela semble être le cas ici, est juste pour que tout le monde sache par où commencer quand on cherche des bugs (ou pour un projet mieux conçu, ce qui peut être disponible pour une réutilisation ultérieure). Quel que soit le résultat de l'examen, il n'y a tout simplement pas le temps de changer quoi que ce soit.

Pour réellement apporter des modifications, vous devez discuter du code et concevoir beaucoup plus tôt dans le projet - le code est beaucoup plus facile à modifier lorsqu'il existe uniquement en tant que discussion sur les approches possibles.

Votre question est "Comment coder un code mal conçu?":

La réponse IMO est simple. Parlez de la conception du code et de la façon dont la conception est défectueuse ou ne répond pas aux exigences. Si vous signalez une conception défectueuse ou "ne répond pas aux exigences", le développeur sera alors contraint de modifier son code car il ne fait pas ce qu'il doit faire.

Si le code est "suffisant du point de vue fonctionnel" et / ou "répond aux spécifications" et / ou "répond aux exigences":

Si vous êtes un pair de ce développeur, vous n’avez aucun pouvoir direct qui vous permettrait de lui "dire" d’apporter des modifications.

Il vous reste quelques options:

1. Vous devez utiliser votre propre "influence" personnelle (une forme de "pouvoir" indirect) et / ou votre capacité à "convaincre"
2. impliquez-vous dans le groupe "processus de code" de votre organisation et commencez à faire de la "maintenance de code" une priorité plus élevée.
3. Mordez la balle et apprenez à lire le code de merde plus rapidement / plus couramment pour ne pas être bloqué (il semble que vous continuiez à être suspendu ou ralenti lorsque vous rencontrez du code de merde) avec du code de merde.
   • Cela fera également de vous un programmeur plus fort.
   • Et cela vous permettra de corriger le code de merde lorsque vous travaillerez sur du code de merde.
   • Et cela vous permettra également de travailler sur plus de projets car de nombreux projets ont un code de merde qui est fonctionnel ... mais beaucoup de code de merde.
4. Mener par l'exemple. Améliorez votre code ... mais n'essayez pas d'être un perfectionniste.
   • Parce que vous deviendrez alors "le type lent qui ne peut pas respecter les délais, critique toujours et pense qu'il est meilleur que les autres".

Je trouve qu'il n'y a pas de solution miracle. Vous devez utiliser les trois et vous devez être créatif dans votre utilisation des trois.

En cas de mauvaise conception, votre objectif devrait être de maximiser l’ encapsulation . De cette façon, il devient plus facile de remplacer les classes / fichiers / sous-routines individuels par des classes mieux conçues.

Veillez à ce que les interfaces publiques des composants soient bien conçues et à ce que le fonctionnement interne soit soigneusement dissimulé. De plus, les enveloppes de stockage de données sont essentielles. (Il peut être très difficile de modifier de grandes quantités de données stockées. Par conséquent, si vous obtenez un "saignement d'implémentation" dans d'autres zones du système, vous êtes en difficulté).

Une fois les barrières levées, concentrez-vous sur les composants les plus susceptibles de poser problème.

Répétez jusqu'à la date limite ou jusqu'à ce que le système soit "parfait".

Au lieu de critiquer directement le code de quelqu'un, il est toujours préférable d'être rentable dans nos commentaires lors de l'examen du code.

Une façon que je suis est

1. ce sera optimal si nous le faisons de cette façon.
2. En l’écrivant de cette façon, il fonctionnera plus vite.
3. Votre code sera beaucoup plus lisible si vous faites "ceci" "ceci" et "ceci"

Ces commentaires seront pris au sérieux même si vos échéances approchent. Et sera probablement mis en œuvre dans le prochain cycle de développement.

La révision du code doit être intégrée au cycle de culture et de développement pour fonctionner. Il est peu probable que la planification d'une révision de code volumineuse à la fin du développement de la fonctionnalité X fonctionne. Tout d'abord, il sera plus difficile de faire les changements et quelqu'un risque de se sentir gêné - créant ainsi une résistance à l'activité.

Vous devez avoir des commits précoces et fréquents, associés à des révisions au niveau de la validation. Avec les outils d'analyse de code en place, la plupart des critiques seront rapides. Des outils automatisés d’analyse et de révision de code, tels que FindBugs et PMD , vous aideront à éliminer un grand nombre d’erreurs de conception. Cependant, ils ne vous aideront pas à résoudre les problèmes d’architecture. Vous devez donc disposer d’une conception solide et évaluer le système dans son ensemble.

Augmenter la qualité des critiques de code.

Outre la qualité du code à l'examen, il existe une qualité de l'examen du code lui-même:

• Les modifications proposées sont-elles vraiment une amélioration par rapport à ce qui est présent?
• Ou juste une question d'opinion?
• À moins que quelque chose de très évident, l'examinateur a bien expliqué, pourquoi ?
• Combien de temps at-il fallu? (J'ai vu des critiques durer des mois, avec le développeur en cours de révision en charge de la résolution de tous les nombreux conflits de fusion).
• Ton?

Il est beaucoup plus facile d’accepter une révision de code de bonne qualité que certaines pratiques de remise en question de l’ego, pour la plupart douteuses.

Il y a deux problèmes notables dans la question, la partie délicate et la prochaine échéance . Il s’agit de problèmes distincts: le premier est une question de communication et de dynamique d’équipe, le second est une question de planification et de hiérarchisation.

Avec tact . Je suppose que vous voulez éviter les égos brossés et le refoulement négatif des critiques. Quelques suggestions:

• Avoir une compréhension commune des normes de codage et des principes de conception.
• Ne jamais critiquer ou revoir le développeur , seulement le code . Évitez le mot "vous" ou "votre code", parlez simplement du code à l'étude, détaché de tout développeur.
• Mettez votre fierté dans la qualité du code terminé , de sorte que les commentaires de révision permettant d'améliorer le résultat final soient appréciés.
• Proposer des améliorations plutôt que la demande. Ayez toujours un dialogue si vous êtes en désaccord. Essayez de comprendre l'autre point de vue lorsque vous n'êtes pas d'accord.
• Demandez aux avis d'aller dans les deux sens. C'est-à-dire que la personne que vous avez révisée revoit votre code ensuite. Ne pas avoir des critiques "à sens unique".

La deuxième partie est la priorisation . Vous avez de nombreuses suggestions d’amélioration, mais comme la date limite approche, il ne reste que le temps d’en appliquer quelques-unes.

Eh bien, vous voulez d’abord éviter que cela se produise! Pour ce faire, vous effectuez des révisions incrémentielles continues. Ne laissez pas un développeur travailler pendant des semaines sur une fonctionnalité, puis relisez-le au dernier moment. Deuxièmement, les révisions de code et le temps nécessaire pour mettre en œuvre les suggestions de révision devraient faire partie de la planification et des estimations régulières pour chaque tâche. S'il n'y a pas assez de temps pour examiner correctement, il y a eu un problème de planification.

Mais supposons que quelque chose a mal tourné dans le processus, et vous êtes maintenant confronté à un certain nombre de commentaires de révision, et vous n'avez pas le temps de les mettre tous en œuvre. Vous devez donner la priorité. Ensuite, optez pour les changements qui seront les plus difficiles et les plus risqués à changer plus tard si vous le retardez.

Nommer des identifiants dans le code source est extrêmement important pour la lisibilité et la maintenabilité, mais il est également assez facile et peu risqué de changer à l'avenir. Idem avec le formatage du code. Alors ne vous concentrez pas sur ce genre de choses. Par ailleurs, la santé des interfaces exposées publiquement devrait être la priorité absolue, car elles sont vraiment difficiles à modifier à l'avenir. Les données persistantes sont difficiles à modifier - si vous commencez par stocker des données incohérentes ou incomplètes dans une base de données, il est très difficile de les corriger à l'avenir.

Les zones couvertes par les tests unitaires présentent un risque faible. Vous pouvez toujours les réparer plus tard. Les zones qui ne le sont pas, mais qui pourraient être testées par unité, présentent un risque plus faible que les zones qui ne peuvent pas être testées par unité.

Supposons que vous disposiez d'une grande quantité de code sans tests unitaires et que vous rencontriez toutes sortes de problèmes de qualité du code, notamment une dépendance codée en dur à un service externe. En injectant plutôt cette dépendance, vous rendez le bloc de code testable. Cela signifie qu’à l’avenir, vous pourrez ajouter des tests puis travailler à résoudre le reste des problèmes. Avec la dépendance codée en dur, vous ne pouvez même pas ajouter de tests. Alors allez-y d'abord pour ce correctif.

Mais s'il vous plaît essayez d'éviter de vous retrouver dans ce scénario en premier lieu!