Que faire lorsque le code soumis pour révision semble trop compliqué?

115

Le code est difficile à suivre, mais il semble (généralement) bien fonctionner, du moins avec des tests superficiels. Il peut y avoir de petits bugs ici et là, mais il est très difficile de dire en lisant le code s'ils sont symptomatiques de problèmes plus profonds ou de simples solutions. Vérifier manuellement l'exactitude globale via la révision du code est cependant très difficile et prend du temps, voire du tout.

Quel est le meilleur plan d'action dans cette situation? Insister sur un do-over? Do-over partiel? Re-factoriser en premier? Résoudre les bugs uniquement et accepter la dette technique ? Effectuez une évaluation des risques sur ces options et décidez ensuite? Autre chose?

Brad Thomas
la source
4
Apparaît ou est? Une mesure rapide des fichiers sources confirmera ou infirmera vos soupçons. Après la mesure, il suffit d’afficher les numéros de mesure lors de la révision du code et de suggérer une refactorisation pour réduire les nombres de complexité.
Jon Raynor
4
Veuillez définir "trop ​​compliqué". Le code est-il trop compliqué parce qu'il utilise des modèles de conception bien connus qui ne sont tout simplement pas familiers à votre équipe ou parce qu'il n'utilise pas de modèles bien connus de votre équipe? Les raisons précises de juger le code «trop compliqué» sont essentielles pour pouvoir évaluer correctement la marche à suivre. Une déclaration aussi simple que "trop ​​compliquée" sur un domaine de connaissances aussi profond et compliqué que l'examen de code suggère une chasse aux sorcières du développeur.
Pieter Geerkens
7
@PieterGeerkens Ou, peut-être, est-ce trop compliqué parce que cela résout un problème compliqué?
Casey

Réponses:

251

S'il ne peut pas être examiné, il ne peut pas passer l'examen.

Vous devez comprendre que la révision de code n'est pas destinée à la recherche de bogues. C'est à cela que sert l'assurance qualité. L'examen du code permet de s'assurer que la maintenance future du code est possible. Si vous ne pouvez même pas suivre le code maintenant, comment pouvez-vous, dans six mois, apporter des améliorations aux fonctionnalités et / ou des corrections de bugs? Trouver des bogues maintenant n’est qu’un avantage secondaire.

Si c'est trop complexe, cela viole une tonne de principes SOLID . Refactor, refactor, refactor. Découpez-le en fonctions bien nommées qui font beaucoup moins, plus simplement. Vous pouvez le nettoyer et vos scénarios de test veilleront à ce qu'il continue de fonctionner correctement. Vous avez des cas de test, non? Sinon, vous devriez commencer à les ajouter.

Kevin Fee
la source
49
C'est beaucoup. N'oubliez pas que ce n'est pas seulement vous et l'auteur qui lirez ce code. Ce sera aussi un stagiaire aléatoire dans 10 ans, vous voulez donc vous assurer qu'il a une chance de pouvoir comprendre ce qui se passe.
David Grinberg
2
bonne réponse. cela dépend de l'objectif de "révision du code". la lisibilité est une chose, la structure une autre - mais leur relation est très étroite. fwiw Je travaille avec un logiciel open source écrit par MAJOR Corps, et il est presque illisible car les noms var et fn sont si compliqués.
19
@ David Grinberg Pour des raisons pratiques, "vous dans six mois" est une personne complètement différente.
Chrylis -on strike-
2
Rangez le code pendant un certain temps (assez longtemps pour qu'il ne se souvienne plus de tout). Demandez au codeur d'origine de l'examiner. Voir si il le comprend.
Nelson
4
Je ne suis pas d'accord avec le fait que l'examen de code "ne soit pas" pour la recherche de bogues. Il trouve souvent des bogues, ce qui constitue un aspect très puissant et utile des critiques de code. Mieux encore, il aide à trouver des moyens d'éviter complètement les bogues dans le futur code. Le point est peut-être exagéré, et devrait être que ce n'est pas exclusivement pour trouver des bugs!
Cody Grey
45

Tout ce que vous mentionnez est parfaitement valide à signaler dans une révision de code.

Lorsque je reçois une révision de code, je vérifie les tests. Si les tests ne fournissent pas une couverture suffisante, c'est quelque chose à souligner. Les tests doivent être utiles pour garantir que le code fonctionne comme prévu et continuera de fonctionner comme prévu dans les modifications. En fait, c'est l'une des premières choses que je recherche dans une révision de code. Si vous n'avez pas prouvé que votre code répond aux exigences, je ne veux pas investir mon temps à le regarder.

Une fois qu'il y a suffisamment de tests pour le code, si le code est complexe ou difficile à suivre, c'est également quelque chose que les humains devraient examiner. Les outils d’analyse statique peuvent indiquer certaines mesures de la complexité et signaler des méthodes trop complexes, ainsi que la découverte d’erreurs potentielles dans le code (et doivent être exécutés avant une révision du code humain). Mais le code est lu et maintenu par des humains, et doit être écrit pour la maintenabilité en premier. Ce n'est que s'il existe une raison d'utiliser un code moins facile à maintenir qu'il devrait être écrit de cette manière. Si vous avez besoin d'un code complexe ou non intuitif, expliquez-le (de préférence dans le code) pourquoi le code est ainsi et proposez des commentaires utiles aux futurs développeurs pour qu'ils comprennent pourquoi et ce qu'il fait.

Idéalement, rejetez les révisions de code qui ne comportent pas de tests appropriés ou dont le code est trop complexe sans raison valable. Il peut y avoir des raisons d’affaires pour aller de l’avant, et pour cela, vous devez évaluer les risques. Si vous contractez une dette technique dans le code, insérez immédiatement des tickets dans votre système de suivi des bogues, avec quelques détails sur ce qui doit être changé et des suggestions pour le changer.

Thomas Owens
la source
30

Vérifier manuellement l'exactitude globale via la révision du code est cependant très difficile et prend du temps, voire du tout.

Ce n'est pas à distance le but d'une révision de code. La façon de penser à une révision de code est d'imaginer qu'il y a un bogue dans le code et que vous devez le corriger. Avec cet état d'esprit, parcourez le code (en particulier les commentaires) et posez-vous la question "Est-il facile de comprendre la grande image de ce qui se passe afin que je puisse circonscrire le problème?" Si c'est le cas, c'est un laissez-passer. Sinon, c'est un échec. Il faut au moins davantage de documentation, ou éventuellement une refactorisation est nécessaire pour rendre le code raisonnablement compréhensible.

Il est important de ne pas être perfectionniste à moins que vous ne soyez sûr que c'est ce que recherche votre employeur. La plupart des codes sont si nuls qu’ils pourraient facilement être remaniés 10 fois de suite, devenant de plus en plus lisibles. Mais votre employeur ne veut probablement pas payer pour avoir le code le plus lisible au monde.

DepressedDaniel
la source
4
Super commentaire! "La plupart des codes sont si nuls qu'ils pourraient facilement être refactorisés 10 fois de suite, de plus en plus lisibles" Bon sang, ai-je été coupable de l'avoir fait :)
Dean Radcliffe
1
"La plupart des codes sont si nuls qu’ils pourraient facilement être refactorisés 10 fois de suite, devenant de plus en plus lisibles." En effet, c'est comme ça dans le monde réel.
Peter Mortensen
@PeterMortensen Il est en effet vrai que vous trouvez beaucoup de cela dans le monde réel. Mais il n’est dans l’intérêt de personne d’avoir un code écrit de cette façon. Je pense qu'il y a deux raisons à cela. Les développeurs reçoivent très peu d’efforts pour apprendre à écrire du code lisible. Et dans certaines entreprises, cela est perçu comme une perte de temps: "Si le développeur a déjà écrit un code fonctionnel, pourquoi devrions-nous veiller à ce qu'il soit lisible ou non? Envoyez simplement la chose."
Kasperd
15

Vérifier manuellement l'exactitude globale via la révision du code est cependant très difficile et prend du temps, voire du tout.

Il y a de nombreuses années, mon travail consistait précisément à classer les devoirs des élèves. Et alors que beaucoup fournissaient une qualité raisonnable avec un bug ici et là, il y en avait deux qui se démarquaient. Les deux ont toujours soumis du code qui n'avait pas de bugs. Un code soumis que je pouvais lire en haut et en bas à haute vitesse et marquer comme correct à 100% avec zéro effort. L'autre code soumis, WTF après l'autre, réussissait néanmoins à éviter les bogues. Une douleur absolue à marquer.

Aujourd'hui, le second aurait son code rejeté lors d'une révision de code. Si la vérification de l'exactitude est très difficile et prend beaucoup de temps, le code pose problème. Un bon programmeur découvrirait comment résoudre un problème (cela prend du temps X) et avant de le confier à un refactorisateur de code afin que cela ne se contente pas de faire le travail, mais bien évidemment de le faire. Cela prend beaucoup moins de temps que X et vous fait gagner beaucoup de temps dans le futur. Souvent, en découvrant les bogues avant même qu'ils passent à l'étape de révision du code. Ensuite, en révisant le code beaucoup plus rapidement. Et tout le temps à l'avenir en rendant le code plus facile à adapter.

Une autre réponse a indiqué que le code de certaines personnes pourrait être modifié 10 fois, devenant ainsi plus lisible. C'est juste triste. C'est un développeur qui devrait chercher un travail différent.

gnasher729
la source
Il me faut beaucoup moins de temps pour refactoriser mon code 10 fois, puis pour écrire la première version du code. Si quelqu'un d'autre sait que j'ai effectué ce refactoring, j'ai échoué.
Ian
6

Est-ce que ce code ancien a été légèrement modifié? (100 lignes de code modifiées dans une base de code de 10000 lignes constituent toujours un léger changement) Parfois, il existe des contraintes de temps et les développeurs sont obligés de rester dans un cadre ancien et peu pratique, tout simplement parce qu'une réécriture complète prendrait encore plus de temps et qu'elle dépassait votre budget. . + habituellement, il y a un risque impliqué, qui peut coûter des millions de dollars lorsqu'il est mal évalué. Si c'est du code ancien, dans la plupart des cas, vous devrez vivre avec. Si vous ne le comprenez pas vous-même, parlez-leur et écoutez ce qu'ils disent, essayez de comprendre. Rappelez-vous, il peut être difficile à suivre pour vous, mais convient parfaitement pour les autres. Prends leur parti, vois-le de leur bout.

Est-ce que ce nouveau code ? En fonction des contraintes de temps, vous devriez plaider en faveur de la refactorisation autant que possible. Est-il possible de consacrer plus de temps à la révision du code si nécessaire? Vous ne devriez pas vous chronométrer à 15 minutes, avoir l’idée et passer à autre chose. Si l'auteur a passé une semaine à écrire quelque chose, vous pouvez passer de 4 à 8 heures à l'examiner. Votre but ici est de les aider à se refactorer. Vous ne retournez pas simplement le code en indiquant "refactor. Now". Voyez quelles méthodes peuvent être décomposées, essayez de trouver des idées pour introduire de nouvelles classes, etc.

Neolisk
la source
2
Vous ne renvoyez pas simplement le code en indiquant "refactor. Now" - pourquoi? J'ai reçu de tels commentaires au moins une fois et la dernière fois que je me souviens, cela s'est avéré utile et correct. Je devais réécrire un gros morceau de code à partir de zéro et c'était la bonne chose à faire car, rétrospectivement, je me suis aperçu que l'ancien code était un gâchis irrécupérable. Avis était tout simplement assez qualifié pour constater que (et je n'a apparemment pas été)
moucheron
4
@gnat: d'abord parce que c'est impoli. Vous semblez mieux lorsque vous expliquez ce qui ne va pas avec le code et que vous faites des efforts pour aider l'autre personne à l'améliorer. Dans une grande entreprise, le faire autrement peut vous faire sortir rapidement de la porte. Surtout si vous révisez le code d'une personne plus âgée.
Neolisk
Dans ce cas, c’est ce que j’ai évoqué plus haut, c’est dans une grande entreprise bien établie qu’il faut faire assez attention à ne pas laisser sortir leurs développeurs les plus qualifiés, du moins pas pour des raisons de partage direct de leurs compétences techniques
Demain,
1
@gnat: L'approche "refactor. now" peut fonctionner à la baisse, c'est-à-dire lorsqu'un développeur senior avec plus de 10 ans d'expérience dit de refactoriser un développeur junior qui a été embauché il y a un mois sans expérience ou situation similaire. Vers le haut - vous pouvez avoir un problème. Comme vous ne connaissez peut-être pas l'expérience de l'autre développeur, il est prudent d'assumer le respect en tant que comportement par défaut. Cela ne vous fera pas de mal à coup sûr.
Neolisk
1
@Neolisk: un développeur expérimenté qui a dû écrire du code avec une contrainte de temps et qui sait qu'il n'est pas assez bon peut être très heureux si vous le refusez, ce qui lui donne du temps et une excuse pour l'améliorer. Le PHB décidant que c'est assez bon rend le développeur malheureux; le critique décidant que ce n'est pas assez bon le rend heureux.
gnasher729
2

Souvent, les correctifs / listes de modifications "complexes" sont ceux qui font beaucoup de choses différentes à la fois. Il y a un nouveau code, un code supprimé, un code refactorisé, un code déplacé, des tests étendus; il est difficile de voir la grande image.

Un indice commun est que le correctif est énorme mais sa description est minuscule: "Implement $ FOO".

Un moyen raisonnable de gérer un tel patch consiste à demander qu’il soit divisé en une série de morceaux plus petits et autonomes. Tout comme le principe de responsabilité unique stipule qu'une fonction ne doit faire qu'une seule chose, un patch doit également se concentrer sur une seule chose.

Par exemple, les premiers correctifs peuvent contenir des modifications de refactorisation purement mécaniques qui ne font pas de modifications fonctionnelles. Les correctifs finaux peuvent ensuite se concentrer sur la mise en œuvre et les tests réels de $ FOO avec moins de distractions et de faussetures.

Pour les fonctionnalités qui nécessitent beaucoup de nouveau code, le nouveau code peut souvent être introduit dans des fragments testables qui ne modifient pas le comportement du produit jusqu'à ce que le dernier correctif de la série appelle réellement le nouveau code (un flip d'indicateur).

Pour ce qui est de faire cela avec tact, je le considère généralement comme mon problème, puis je demande l'aide de l'auteur: "J'ai du mal à suivre tout ce qui se passe ici. Pourriez-vous diviser ce patch en étapes plus petites pour m'aider à comprendre comment ensemble?" Il est parfois nécessaire de faire des suggestions spécifiques pour les plus petites étapes.

Si gros patch comme "Implement $ FOO" se transforme en une série de patchs comme:

  1. Introduisez une nouvelle version de Frobnicate qui nécessite deux itérateurs, car je vais devoir l'appeler avec des séquences autres que vector pour implémenter $ FOO.
  2. Basculez tous les appelants existants de Frobnicate pour utiliser la nouvelle version.
  3. Supprimez l'ancien Frobnicate.
  4. Frobnicate en faisait trop. Intégrez l’étape la plus simple dans votre propre méthode et ajoutez des tests pour cela.
  5. Présentez Zerzify, avec des tests. Pas encore utilisé, mais j'en aurai besoin pour $ FOO.
  6. Implémentez $ FOO avec Zerzify et le nouveau Frobnicate.

Notez que les étapes 1 à 5 n'apportent aucune modification fonctionnelle au produit. Ils sont faciles à examiner, notamment pour vous assurer que vous avez tous les bons tests. Même si l'étape 6 est toujours "compliquée", au moins, elle est axée sur $ FOO. Et naturellement, le journal vous donne une bien meilleure idée de la façon dont $ FOO a été implémenté (et de la raison pour laquelle Frobnicate a été modifié).

Adrian McCarthy
la source
Une approche, si vous utilisez Git, consiste à composer une demande d'extraction de plusieurs commits. Chaque commit est aussi atomique et autonome que possible, et a sa propre description. Ajoutez ensuite une note utile dans le corps du PR indiquant que chaque modification peut être examinée manuellement. C'est généralement ainsi que je gère de très gros PR, comme des refactors globaux ou de grands changements d'outillage inattaquables.
Jimmy Breck-McKye
1

Comme d'autres l'ont souligné, la révision de code n'est pas vraiment conçue pour rechercher des bogues. Si vous rencontrez des problèmes lors de la révision du code, cela signifie probablement que votre couverture de tests automatisés est insuffisante (par exemple, tests unitaires / d'intégration). S'il n'y a pas assez de couverture pour me convaincre que le code fait ce qu'il est censé faire , je demande généralement plus de tests et indique le type de tests que je recherche et n'autorise généralement pas le code dans la base de code qui n'a pas une couverture adéquate. .

Si l'architecture de haut niveau est trop complexe ou n'a pas de sens, j'organise généralement une réunion avec quelques membres de l'équipe pour en discuter. Il est parfois difficile d'itérer sur une mauvaise architecture. Si le développeur était un novice, je m'assure généralement que nous examinions sa pensée à l' avance au lieu de réagir à une mauvaise demande de tirage. Cela est généralement vrai même avec des développeurs plus expérimentés si le problème n'a pas de solution évidente qui sera probablement choisie.

Si la complexité est isolée au niveau de la méthode, cela peut généralement être corrigé de manière itérative et avec de bons tests automatisés.

Un dernier point. En tant que réviseur, vous devez décider si la complexité du code est due à une complexité essentielle ou accidentelle . La complexité essentielle concerne les parties du logiciel qui sont légitimement difficiles à résoudre. La complexité accidentelle fait référence à toutes les autres parties du code que nous écrivons qui est trop complexe pour aucune raison et qui pourrait être facilement simplifiée.

Je m'assure généralement que le code avec la complexité essentielle est vraiment cela et ne peut pas être simplifié davantage. Je vise également une plus grande couverture de test et une bonne documentation pour ces parties. La complexité accidentelle devrait presque toujours être nettoyée au cours du processus de demande d'extraction, car ils constituent l'essentiel du code que nous traitons et peuvent facilement causer un cauchemar de maintenance, même à court terme.

c_maker
la source
0

A quoi ressemblent les tests? Ils doivent être clairs, simples et faciles à lire avec idéalement une seule affirmation. Les tests doivent clairement documenter le comportement souhaité et les cas d'utilisation du code.

Si ce n'est pas bien testé, c'est un bon endroit pour commencer à examiner.

Tom Squires
la source