Que dois-je faire en tant que réviseur lorsque le code «doit être» mauvais?

Je travaille sur un projet mal conçu et sur-conçu où des révisions de code obligatoires ont été introduites il y a quelques mois.

On m'a demandé de revoir un morceau de code substantiel qui implémente une nouvelle fonctionnalité. Il présente les mêmes défauts que le reste de notre base de code. Je comprends que dans une large mesure, ces défauts se glissent dans le nouveau code, par exemple. en ayant à hériter d'une classe mal conçue ou à implémenter une interface mal conçue tout en conservant la compatibilité, et je ne peux pas vraiment offrir une meilleure solution qui n'impliquerait pas la réécriture de la moitié de la base de code. Mais je pense que du point de vue de l'ingénierie, cela ne sert à rien à une base de code déjà cassée que nous la cassions encore plus. Le code que je revois est définitivement mauvais, mais il doit l'être si la fonctionnalité doit être implémentée.

Comment dois-je me comporter par rapport à cet examen particulier? Existe-t-il un moyen pour moi de maintenir l'intégrité et de rester constructif?

Veuillez noter que je pose des questions sur la limite de l'examen du code dans ce contexte. Je me rends compte que le problème est causé par d'autres facteurs, notamment l'organisation et la culture de travail, mais ce que je veux savoir, c'est comment gérer l'examen lui-même.

Facile:

1. Documentez votre dette technique

Vous avez identifié un morceau de code qui fonctionne mais qui présente des problèmes techniques. Il s'agit d' une dette technique . Comme d'autres types de dette, elle s'aggrave avec le temps si elle n'est pas réglée.

Cette première étape dans le traitement de la dette technique consiste à le documenter. Pour ce faire, ajoutez des éléments dans votre outil de suivi des problèmes qui décrivent la dette. Cela vous aidera à obtenir une image plus claire de l'ampleur du problème et vous aidera également à élaborer un plan pour y faire face.

2. Remboursez progressivement votre dette

Modifiez votre processus de développement logiciel pour prendre en compte le remboursement de la dette technique. Cela peut impliquer un sprint de durcissement occasionnel, ou simplement résoudre des éléments de dette à un intervalle régulier (par exemple, 2 éléments par semaine). La partie importante pour vous assurer que vous réduisez votre dette plus rapidement que sa dette (la dette, même la dette technique, a des intérêts).

Une fois que vous avez atteint un point où vous n'avez plus de déficit technique, vous êtes en route vers une base de code plus saine :)

En remarque: recherchez un nouvel emploi. Celui-ci ne s'améliorerait pas.

Les objectifs du code que vous examinez sont les suivants:

• Pour expédier une fonctionnalité, qui devrait fonctionner selon les exigences.

• Réduire la croissance de la dette technique.

Le premier objectif est revu en vérifiant que les tests unitaires, d'intégration, de système et fonctionnels sont bien là, qu'ils sont pertinents et qu'ils couvrent toutes les situations à tester. Vous devez également vérifier les croyances que l'auteur original peut avoir sur le langage de programmation, ce qui pourrait conduire à des bogues subtils ou au code faisant semblant de faire quelque chose de différent de ce qu'il fait réellement.

Le deuxième objectif est celui sur lequel votre question se concentre. D'une part, le nouveau code ne devrait pas augmenter la dette technique. D'un autre côté, la portée de l'examen est le code lui-même, mais dans le contexte de la base de code entière. À partir de là, vous, en tant que critique, pouvez vous attendre à deux approches de l'auteur original:

• Le code extérieur n'est pas de ma faute. Je viens d'implémenter la fonctionnalité et je ne me soucie pas de la base de code entière.

  Dans cette perspective, le code copiera les défauts de la base de code, et donc augmentera inévitablement la dette technique: plus de mauvais code est toujours pire.

  Bien qu'il s'agisse d'une approche à court terme valable, à long terme, elle entraînerait une augmentation des retards et une faible productivité, et conduirait finalement à un processus de développement si coûteux et risqué que le produit cesserait d'évoluer.

• La rédaction d'un nouveau code est l'occasion de refactoriser l'ancien.

  Dans cette perspective, l'effet des défauts du code hérité sur le nouveau pourrait être limité. De plus, la dette technique pourrait être réduite, ou du moins ne pas augmenter proportionnellement à la croissance du code.

  Bien qu'il s'agisse d'une approche à long terme valable, elle présente des risques à court terme. Le principal est que, à court terme, il faudrait parfois plus de temps pour expédier la fonctionnalité spécifique. Un autre aspect important est que si le code hérité n'est pas testé, sa refactorisation présente un risque énorme d'introduire des régressions.

Selon le point de vue que vous souhaitez encourager, vous pourriez être enclin à conseiller aux réviseurs de refaçonner plus ou non. Dans tous les cas, ne vous attendez pas à un morceau de code impeccable et propre avec une architecture et un design agréables dans une base de code merdique. Ce que vous ne devriez pas encourager, c'est le comportement où un développeur compétent qui doit travailler sur une base de code merdique essaie de bien faire sa part . Au lieu de simplifier les choses, cela ne fait que les rendre plus compliquées qu'avant. Maintenant, au lieu d'un mauvais code uniforme, vous avez une partie avec des modèles de conception, une autre partie avec du code clair et clair, une autre partie qui a été largement refactorisée au fil du temps, et aucune unité du tout.

Imaginez, par exemple, que vous découvrez une base de code héritée d'un site Web de taille moyenne. Vous êtes surpris par l'absence de toute structure habituelle et par le fait que la journalisation, lorsqu'elle est terminée, se fait en ajoutant des éléments à un fichier texte à la main, au lieu d'utiliser un cadre de journalisation. Vous décidez que la nouvelle fonctionnalité utilise MVC et un cadre de journalisation.

Votre collègue met en œuvre une autre fonctionnalité et est très surpris par l'absence d'un ORM où l'on ferait une taille parfaite. Il commence donc à utiliser un ORM.

Ni vous, ni votre collègue ne pouvez parcourir des centaines de milliers de lignes de code pour utiliser MVC, un cadre de journalisation ou un ORM partout. En fait, cela nécessiterait des mois de travail: imaginez l'introduction de MVC; Combien de temps cela prendrait-il? Ou qu'en est-il d'un ORM dans des situations où les requêtes SQL ont été générées de manière chaotique par concaténation (avec des emplacements occasionnels pour l'injection SQL) dans du code que personne ne pouvait comprendre?

Vous pensez que vous avez fait du bon travail, mais maintenant, un nouveau développeur qui rejoint le projet doit faire face à beaucoup plus de complexité qu'auparavant:

• L'ancienne façon de traiter les demandes,

• La façon MVC,

• L'ancien mécanisme de journalisation,

• Le cadre de journalisation,

• L'accès direct à la base de données avec des requêtes SQL construites à la volée,

• L'ORM.

Sur un projet sur lequel je travaillais, quatre (!) Frameworks de journalisation étaient utilisés côte à côte (plus la journalisation manuelle). La raison est que chaque fois que quelqu'un voulait enregistrer des trucs, il n'y avait pas d'approche commune pour le faire, donc au lieu d'apprendre un nouveau cadre (qui dans tous les cas n'était utilisé que dans 5% de la base de code), on en ajoutait simplement un autre qu'il sait déjà. Imaginez le désordre.

Une meilleure approche serait de refactoriser la base de code une étape à la fois. Reprenant l'exemple de la journalisation, le refactoring comprendrait les petites étapes suivantes:

• Recherchez tous les endroits où la journalisation héritée est effectuée (c'est-à-dire lorsque le fichier journal est accessible directement) et assurez-vous qu'ils appellent tous les mêmes méthodes.

• Déplacez ce code vers une bibliothèque dédiée, le cas échéant. Je ne veux pas enregistrer la logique de stockage dans ma classe de panier.

• Modifiez, si nécessaire, l'interface des méthodes de journalisation. Par exemple, nous pouvons ajouter un niveau indiquant si le message est informel ou s'il s'agit d'un avertissement ou d'une erreur.

• Utilisez les méthodes nouvellement refactorisées dans la nouvelle fonctionnalité.

• Migrez vers la structure de journalisation: le seul code affecté est le code de la bibliothèque dédiée.

Si vous effectuez des révisions de code dans le cadre de votre processus de développement; vous devez ensuite définir les règles par rapport auxquelles vous «jugez» le code que vous examinez.

Cela devrait entrer dans votre `` définition du fait '' et pourrait être un guide de style, un document d'architecture pour la base de code ou une analyse statique, vérifiant les exigences légales, tout ce que l'entreprise décide de ses exigences de `` qualité du code ''

Une fois que vous avez ceci en place, les révisions de code sont devenues une évidence, le guide de style a-t-il été suivi, avons-nous le pourcentage requis de couverture de test, etc.

Si vous n'avez pas cela en place, les révisions de code peuvent simplement devenir une bataille pour savoir qui a le plus grand bon vouloir de codage ou, comme dans votre situation, remettre en question les jugements sur le refactoring par rapport aux fonctionnalités effectuées avant la date limite. Ce qui est juste une perte de temps pour tout le monde.

Votre principal problème est qu'une révision de code sur une nouvelle fonctionnalité importante n'est pas le bon moment pour avoir cette discussion. À ce stade, il est trop tard pour apporter autre chose que des changements mineurs. Le bon endroit est au stade de la planification ou au plus tard dans une revue de conception préliminaire. Si votre entreprise ne procède pas à ces premières évaluations au moins de manière informelle, vous devez d'abord travailler à changer cette culture.

La prochaine étape consiste à vous inviter à ces réunions et à avoir des idées productives lors de ces réunions. Surtout, cela signifie ne pas essayer de tout changer du jour au lendemain, mais rechercher des morceaux de la taille d'une bouchée que vous pouvez isoler et attaquer. Ces morceaux s'additionneront considérablement au fil du temps.

En d'autres termes, la clé est de suggérer régulièrement des changements plus modestes vers le début des projets, plutôt que d'être abattu suggérant des changements plus importants vers la fin.

Dans les endroits où j'ai fait la révision du code, j'accepterais et j'accorderais la soumission du code, si elle s'accompagne d'un engagement à faire (au moins) une refactorisation. Que ce soit comme un bogue déposé, comme une histoire, ou comme une promesse d'envoyer une autre critique avec (certains) refactoring fait.

Dans ces cas, si je suis la personne qui écrit le code à réviser, je prépare généralement deux changements, l'un avec mes nouvelles fonctionnalités ou corrections de bogues, puis un autre qui a cela ET un certain nettoyage. De cette façon, les changements de nettoyage ne nuisent pas aux nouvelles fonctionnalités ou correctifs de bogue, mais sont facilement signalés comme un jeton "oui, je sais que cela ne résout pas ces problèmes, mais là-bas" pur nettoyage "qui fait".