La couverture de code met en évidence les méthodes inutilisées - que dois-je faire?

59

J'ai été chargé d'augmenter la couverture de code d'un projet Java existant.

J'ai remarqué que l'outil de couverture de code ( EclEmma ) a mis en évidence certaines méthodes qui ne sont jamais appelées de nulle part.

Ma première réaction n’est pas d’écrire des tests unitaires pour ces méthodes, mais de les mettre en évidence à mon supérieur hiérarchique et à mon équipe et de leur demander pourquoi ces fonctions sont déjà là.

Quelle serait la meilleure approche? Ecrivez des tests unitaires pour eux, ou demandez-vous pourquoi ils sont là?

Lucas T
la source
1
Les commentaires ne sont pas pour une discussion prolongée; cette conversation a été déplacée pour discuter .
maple_shaft

Réponses:

118
  1. Effacer.
  2. Commettre.
  3. Oublier.

Raisonnement:

  1. Le code mort est mort. Par sa description même, il n’a aucun sens. Cela a peut-être eu un but à un moment donné, mais cela a disparu et le code devrait donc disparaître.
  2. Le contrôle de version garantit que, dans (selon mon expérience), un événement unique et rare qui survient plus tard pour rechercher ce code peut être récupéré.
  3. Comme effet secondaire, vous améliorez instantanément la couverture de code sans rien faire (sauf si le code mort est testé, ce qui est rarement le cas).

Mise en garde tirée des commentaires: la réponse initiale supposait que vous aviez bien vérifié que le code était sans aucun doute mort. Les IDE sont faillibles, et il est possible d'appeler du code qui a l'air mort. Faites appel à un expert sauf si vous êtes absolument certain.

l0b0
la source
118
Généralement, je suis d’accord avec cette approche, mais si vous êtes nouveau dans un projet et / ou un développeur junior, demandez à votre mentor / supérieur avant de le supprimer. Selon le projet, certaines méthodes peuvent ne pas sembler être utilisées, mais être appelées / utilisées par du code externe, mais pas dans le code de projet auquel vous avez accès. Il s’agit peut-être d’un code généré automatiquement pour que votre suppression ne produise que du bruit de l’historique des journaux. Votre IDE peut ne pas être en mesure de trouver un accès basé sur la réflexion à partir du projet. Etc. pp. Si vous n'êtes pas sûr de ces affaires, demandez au moins une fois.
Frank Hopkins
11
Bien que je sois d’accord avec le cœur de cette réponse, la question littérale était «devrais-je demander pourquoi ils sont là? . Cette réponse pourrait donner l’impression que le PO ne devrait pas demander à son équipe avant de supprimer.
Doc Brown
58
J'ajouterais toutefois une étape: vérifiez dans le journal des erreurs git pour les lignes de code inutilisées. Si vous pouvez trouver la personne qui les a écrites, vous pouvez demander à quoi elles servent. Si les lignes sont nouvelles, il est fort probable que quelqu'un envisage de les utiliser bientôt (auquel cas elles devraient probablement être testées à l'unité maintenant).
bdsl
10
Il y a tellement de problèmes avec cette réponse, exprimée dans des commentaires ou d'autres réponses, que je ne peux pas croire que ce soit la réponse la plus votée et acceptée.
user949300
11
@ Emmory La meilleure façon de commencer dans une nouvelle équipe est de casser la construction en supprimant des éléments sans le vouloir, car vous pensiez que personne n'en avait besoin. Garantit que vous êtes populaire dès le premier jour. Bien sûr, cela n’est peut-être pas nécessaire (car chaque application volumineuse et ancienne contient toujours une toux de 100% de la couverture de code ), mais c’est un très mauvais retour sur investissement.
Voo
55

Toutes les autres réponses sont basées sur l'hypothèse que les méthodes en question sont vraiment inutilisées. Cependant, la question ne précisait pas si ce projet est autonome ou s'il s'agit d'une bibliothèque.

Si le projet en question est une bibliothèque, les méthodes apparemment inutilisées peuvent être utilisées en dehors du projet et leur suppression briserait ces autres projets. Si la bibliothèque elle-même est vendue à des clients ou rendue publique, il peut même être impossible de localiser l'utilisation de ces méthodes.

Dans ce cas, il y a quatre possibilités:

  • Si les méthodes sont private ou package-private, elles peuvent être supprimées en toute sécurité.
  • Si les méthodes sont publiques, leur présence peut être justifiée même sans utilisation réelle, afin de compléter les fonctionnalités. Ils devraient être testés si.
  • Si les méthodes sont publiques et inutiles, leur suppression constituera un changement radical et si la bibliothèque suit un contrôle de version sémantique , cela n'est autorisé que dans une nouvelle version majeure.
  • Les méthodes publiques peuvent également être obsolètes et supprimées ultérieurement. Cela donne aux utilisateurs d'API un peu de temps pour passer des fonctions déconseillées avant leur suppression dans la prochaine version majeure.
Zoltan
la source
9
De plus, s’il s’agit d’une bibliothèque, les fonctions sont là pour des raisons de complétude
PlasmaHH
Pouvez-vous préciser comment ils devraient être testés? Si vous ne savez pas pourquoi la méthode existe, vous ne savez probablement pas ce qu'elle est censée faire.
Non U
Les noms de méthode tels que filter_name_exists, ReloadSettingsou addToSchema(choisis au hasard dans 3 projets open source arbitraires) devraient fournir quelques indications sur ce que la méthode est supposée faire. Un commentaire javadoc peut être encore plus utile. Je ne sais pas si les spécifications sont correctes, mais cela pourrait suffire à créer quelques tests pouvant au moins empêcher les régressions.
Zoltan
1
les méthodes publiques sur une classe ou une interface privée ne doivent pas être considérées comme publiques à cette fin. De même, si une classe publique est imbriquée dans une classe privée, elle n'est pas vraiment publique.
Emory
30

Commencez par vérifier que votre outil de couverture de code est correct.

J'ai eu des situations où ils n'ont pas compris les méthodes appelées via des références à l'interface, ou si la classe est chargée dynamiquement quelque part.

Ewan
la source
Merci, va faire. Sur Eclipse, je fais une recherche sur la base de code pour la fonction, et rien ne se présente. Si vous avez d'autres suggestions sur la manière de faire une recherche plus complète, je vous en serais très reconnaissant.
Lucas T
3
oui, vous devriez vérifier d'autres projets qui pourraient importer la classe en tant que dll
Ewan
7
Cette réponse est incomplète. "Vérifiez d'abord que votre outil de couverture de code est correct. Si c'est le cas, alors .... [insérer le reste de la réponse] ". Plus précisément, le PO veut savoir quoi faire si l’outil de couverture de code est correct.
Jon Bentley
1
@jonbentley Le PO demande la meilleure approche pour le rapport sur les outils. "Vérifiez-le manuellement" car il est assez évident, compte tenu du contexte, que ce soit incorrect
Ewan,
15

Comme Java est compilé statiquement, il devrait être assez sûr de supprimer les méthodes. Supprimer le code mort est toujours bon. Il existe certaines probabilités qu'il existe un système de réflexion insensé qui les exécute au moment de l'exécution. Vérifiez donc d'abord auprès des autres développeurs, mais supprimez-les.

max630
la source
9
+1 pour vérifier avec les gens, c'est un peu le principe de la moindre surprise. Vous ne voulez pas que quelqu'un passe trop de temps à rechercher la méthode que vous venez de quitter quelques validations plus tôt. De plus, dans certains cas extrêmes, le code mort peut être le nouveau contenu déjà enregistré mais pas encore câblé (mais dans ce cas, il devrait être bien documenté avec des commentaires et testé).
Frax
4
Eh bien, sauf si le code utilise la réflexion pour appeler les méthodes "inutilisées", mais dans ce cas, vous avez des problèmes beaucoup plus importants, et nous attendons avec impatience de voir le code sur thefailywft.com (Faites un test rapide pour voir si un code utilise un ClassName. classe).
MT Whileed
2
Il est compilé statiquement, mais il y a aussi l' invokedynamicinstruction, alors,
tu
@MTilsted: Il existe de nombreux frameworks qui appellent du code en utilisant des chaînes - je peux au moins penser à Spring et à Hibernate par cœur.
Jhominal
9

Quelle serait la meilleure approche? Ecrivez des tests unitaires pour eux, ou demandez-vous pourquoi ils sont là?

La suppression de code est une bonne chose.

Lorsque vous ne pouvez pas supprimer le code, vous pouvez certainement le marquer comme @Deprecated , en indiquant la version majeure que vous ciblez pour supprimer la méthode. Ensuite, vous pouvez le supprimer "plus tard". Entre temps, il sera clair qu’aucun nouveau code ne dépend de celui-ci.

Je ne recommanderais pas d'investir dans des méthodes obsolètes - donc pas de nouveaux tests unitaires juste pour atteindre les objectifs de couverture.

La différence entre les deux est principalement de savoir si les méthodes font ou non partie de l' interface publiée . La suppression arbitraire de parties de l'interface publiée peut être une surprise désagréable pour les consommateurs qui dépendaient de l'interface.

Je ne peux pas parler à EclEmma, ​​mais d'après mes expériences, vous devez faire attention à la réflexion. Si, par exemple, vous utilisez des fichiers de configuration de texte pour choisir les classes / méthodes auxquelles accéder, la distinction utilisé / non utilisé peut ne pas être évidente (j'ai été gravé par ce moyen avec des temps couplés).

Si votre projet est une feuille dans le graphique de dépendance, le cas de dépréciation est alors affaibli. Si votre projet est une bibliothèque, les arguments en faveur de la dépréciation sont plus justes.

Si votre société utilise une prise en pension mono , la suppression présente un risque plus faible que dans le cas d'une prise en pension multiple.

Comme indiqué par l0b0 , si les méthodes sont déjà disponibles dans le contrôle de source, leur récupération après suppression est un exercice simple. Si cela vous inquiète vraiment, réfléchissez à la manière d’organiser vos commits de manière à pouvoir récupérer les modifications supprimées si vous en avez besoin.

Si l'incertitude est suffisante, vous pouvez envisager de commenter le code plutôt que de le supprimer. C'est un travail supplémentaire dans le chemin heureux (où le code supprimé n'est jamais restauré), mais cela facilite la restauration. Mon hypothèse est que vous devriez préférer une suppression directe jusqu'à ce que vous ayez été brûlé plusieurs fois, ce qui vous donnera quelques indications sur la façon d'évaluer "l'incertitude" dans ce contexte.

question pourquoi ils sont là?

Le temps investi pour capturer la tradition n'est pas nécessairement perdu. On m'a appris à effectuer une suppression en deux étapes - tout d'abord, en ajoutant et en validant un commentaire expliquant ce que nous avons appris sur le code, puis en supprimant ultérieurement le code (et le commentaire).

Vous pouvez également utiliser quelque chose d'analogue aux enregistrements de décisions architecturales comme moyen de capturer la tradition avec le code source.

VoiceOfUnreason
la source
9

Un outil de couverture de code n'est pas omniscient, ni omniprésent. Ce n'est pas parce que votre outil prétend que la méthode n'est pas appelée qu'elle ne s'appelle pas . Il y a une réflexion, et selon la langue, il peut y avoir d'autres moyens d'appeler la méthode. En C ou C ++, il peut y avoir des macros qui construisent des noms de fonctions ou de méthodes, et l'outil peut ne pas voir l'appel. La première étape serait donc: Effectuez une recherche textuelle du nom de la méthode et des noms associés. Demandez à des collègues expérimentés. Vous pouvez trouver qu'il est réellement utilisé.

Si vous n’êtes pas sûr, mettez un assert () au début de chaque méthode "non utilisée". Peut-être qu'on l'appelle. Ou une déclaration de journalisation.

Peut-être que le code est réellement précieux. C’est peut-être un nouveau code sur lequel un collègue travaille depuis deux semaines et qu’il allait s’allumer demain. Ce n'est pas appelé aujourd'hui parce que l'appel sera ajouté demain.

Peut-être le code est-il réellement utile, partie 2: le code peut exécuter des tests d’exécution très coûteux qui pourraient permettre de détecter des problèmes. Le code n'est activé que si les choses tournent mal. Vous supprimez peut-être un outil de débogage précieux.

Fait intéressant, le pire conseil possible "Supprimer. Engager. Oublier." est le mieux noté. (Revues de code? Vous ne faites pas de revues de code? Que faites-vous pour programmer si vous ne faites pas de critiques de code?)

gnasher729
la source
Je ne suis pas du tout d'accord avec le fait que "SUPPRIMER. ENGAGER. OUBLIER" soit le pire conseil. (Je pense que c'est le meilleur.) Votre approche est également OK. Je pense que le pire conseil possible serait d'écrire des tests unitaires qui exercent le "code mort" mais ne font aucune affirmation. L'outil de couverture de code sera trompé en pensant qu'ils sont utilisés.
Emory
3
Rien dans la réponse "Supprimer. Engager. Oublier" ne dit pas de réviser le code. Il est parfaitement possible (et conseillé, à mon humble avis) de réviser le code une fois qu'il a été validé (mais avant le déploiement :-)).
Sleske
@sleske Comment examinez-vous le code qui n'existe plus? Cette réponse ne mentionne pas non plus "révision".
user949300
@ user949300: Qu'une modification de code soit soumise ou non à une révision constitue une question distincte, qu'elle soit ajoutée, modifiée ou supprimée (l'ajout de code peut être encore plus désastreux que sa suppression, voir par exemple la vulnérabilité Heartbleed). De plus, la réponse (maintenant) indique de "faire appel à un expert", ce qui semble bien proche d'une révision de code.
Sleske
1
@ user949300: En ce qui concerne "Comment révisez-vous le code qui n'est plus là?" - J'espère que c'est évident: regardez l'évolution de l'outil de contrôle de version de votre choix. Sinon, comment réviseriez-vous les modifications (éventuelles modifications)?
Sleske
6

Selon l'environnement dans lequel le logiciel s'exécute, vous pouvez vous connecter si la méthode est appelée. Si elle n'est pas appelée dans un délai approprié, la méthode peut être supprimée en toute sécurité.

Cette approche est plus prudente que la simple suppression de la méthode et peut s'avérer utile si vous utilisez une environnement extrêmement sensible aux erreurs.

Nous nous connectons à un #unreachable-codecanal slack dédié avec un identifiant unique pour chaque candidat à supprimer, et il s'est avéré très efficace.

Jamie Bull
la source
Mais que se passe-t- il si on l’appelle rarement (comme un 1 sur 1440 en moyenne) ?
Peter Mortensen
alors "une période de temps convenable" est plus longue (bien que j'aime bien un "homme après minuit")
Jamie Bull