Nom pour cet antipattern? Champs en tant que variables locales [fermé]

68

Dans certains codes que je suis en train de réviser, je vois des choses qui sont l'équivalent moral de ce qui suit:

public class Foo
{
    private Bar bar;

    public MethodA()
    {
        bar = new Bar();
        bar.A();
        bar = null;
    }

    public MethodB()
    {
        bar = new Bar();
        bar.B();
        bar = null;
    }
}

Le champ barici est logiquement une variable locale, car sa valeur n'est jamais destinée à persister dans les appels de méthodes. Cependant, comme beaucoup de méthodes Foonécessitent un objet de type Bar, l'auteur du code original vient de créer un champ de type Bar.

  1. C'est évidemment mauvais, non?
  2. Y a-t-il un nom pour cet antipattern?
JSB ձոգչ
la source
60
Eh bien, c'est un nouveau. J'espère que cette classe ne sera pas utilisée dans un contexte multithread, ou vous aurez des temps intéressants à venir!
TMN
8
C'est vraiment rare? J'ai vu cela plusieurs fois dans ma carrière.
JSB
32
@TMN Ce n'est pas thread-safe, mais ce qui est pire, ce n'est même pas réentrant. Si tout code MethodAou la MethodBcause MethodAou MethodBà appeler de quelque façon (et barest utilisé à nouveau, dans le cas de l' bar.{A,B}()être le délinquant) vous avez des problèmes similaires , même sans aucune concurrence.
73
Devons-nous avoir un nom pour chaque chose stupide que quelqu'un pourrait faire? Appelez ça une mauvaise idée.
Caleb
74
Difficile de ne pas l'appeler l'anti-modèle privé suspendu.
psr

Réponses:

86

C'est évidemment mauvais, non?

Oui.

  • Cela rend les méthodes non-réentrantes, ce qui pose un problème si elles sont appelées sur la même instance de manière récursive ou dans un contexte multithread.

  • Cela signifie que l'état d'un appel se transforme en un autre appel (si vous oubliez de réinitialiser).

  • Cela rend le code difficile à comprendre car vous devez vérifier ce qui précède pour être sûr de ce que le code fait réellement. (Contraste avec l'utilisation de variables locales.)

  • Cela rend chaque Fooinstance plus grande que nécessaire. (Et imaginez le faire pour N variables ...)

Y a-t-il un nom pour cet antipattern?

OMI, cela ne mérite pas d'être appelé un anti-modèle. C'est juste une mauvaise habitude de codage / une mauvaise utilisation des constructions Java / code de la merde. C'est le genre de chose que vous pourriez voir dans le code d'un étudiant de premier cycle lorsque l'étudiant a sauté des conférences et / ou n'a aucune aptitude à la programmation. Si vous le voyez dans le code de production, c'est un signe que vous devez faire beaucoup plus de révisions de code ...


Pour mémoire, la manière correcte de l'écrire est la suivante:

public class Foo
{
    public methodA()
    {
        Bar bar = new Bar();  // Use a >>local<< variable!!
        bar.a();
    }

    // Or more concisely (in this case) ...
    public methodB()
    {
        new Bar().b();
    }
}

Notez que j'ai également corrigé les noms de méthodes et de variables afin qu'ils soient conformes aux règles de style acceptées pour les identificateurs Java.

Stephen C
la source
11
Je dirais "Si vous le voyez dans le code de production, c'est un signe que vous devriez réexaminer votre processus d'embauche "
Version bêta
@Beta - cela aussi, même si vous devriez être capable de corriger de mauvaises habitudes de codage comme cela avec une révision de code vigoureuse.
Stephen C
+1 pour le bit sur plus de critiques de code. Malgré ce que les gens pensent, améliorer les pratiques d’embauche n’empêchera pas ce type de problème: l’embauche est une foutaise, le mieux que vous puissiez faire est de lancer les dés en votre faveur.
mattnz
@StephenC "Cela rend les méthodes non-réentrantes, ce qui pose un problème si elles sont appelées sur la même instance de manière récursive ou dans un contexte multithread." . Je sais ce qu'est la réentrance, mais je ne vois pas le moindre point pour cela car les méthodes ne font pas appel à des verrous? Pouvez-vous expliquer s'il vous plaît.
Geek
@ Geek - Vous vous concentrez trop sur l'exemple spécifique. Je parle du cas général où la méthode peut être appelée à partir de plusieurs threads, ou de manière récursive.
Stephen C
39

J'appellerais cela une grande portée inutile pour une variable. Ce paramètre autorise également des conditions de concurrence si plusieurs threads accèdent à MethodA et à MethodB.

Les 00 heures
la source
12
Plus précisément, je pense que "portée variable" est le nom du problème ici. Cette personne doit apprendre quelles sont les variables locales et comment / quand les utiliser. Le modèle positif ou règle générale consiste à utiliser la plus petite portée disponible pour chaque variable et à l'élargir au besoin. Pour être clair, cette erreur n'est pas seulement un mauvais style. Coder une condition de concurrence comme celle-ci est une erreur.
GlenPeterson
30

Ceci est un cas spécifique d'un modèle général connu sous le nom de global doorknobbing. Lors de la construction d'une maison, il est tentant d'acheter un seul bouton de porte et de le laisser traîner. Quand quelqu'un veut utiliser une porte ou un placard, il ne fait que saisir cette poignée de porte globale. Si les poignées de porte sont chères, cela peut être un bon design.

Malheureusement, lorsque votre ami arrive et que la poignée de porte est utilisée simultanément à deux endroits différents, la réalité se brise. Si la poignée de porte est si chère que vous ne pouvez en acheter qu'une, cela vaut la peine de mettre en place un système qui permette aux gens d'attendre leur tour pour la porte.

Dans ce cas, la poignée de porte est juste une référence, donc c'est bon marché. Si la poignée de porte était un objet réel (et qu'ils réutilisaient l'objet au lieu de simplement la référence), il pourrait alors être assez coûteux pour être un prudent global doorknob. Cependant, quand il est bon marché, il est connu sous le nom de cheapskate global doorknob.

Votre exemple est de la cheapskatevariété.

Ned Twigg
la source
19

La préoccupation principale ici serait la concurrence - Si le Foo est utilisé par plusieurs threads, vous avez un problème.

Au-delà, c’est idiot: les variables locales gèrent parfaitement leur propre cycle de vie. Les remplacer par des variables d'instance qui doivent être annulées lorsqu'elles ne sont plus utiles est une invitation à l'erreur.

Matthew Flynn
la source
15

C'est un cas spécifique d '"étendue inappropriée", avec un côté "réutilisation variable".

Ikegami
la source
7

Ce n'est pas un anti-modèle. Les anti-modèles ont certaines propriétés qui en font une bonne idée, ce qui pousse les gens à le faire volontairement; ils sont planifiés comme des motifs et ensuite ça va terriblement mal.

C'est aussi ce qui fait que l'on discute du fait de savoir si quelque chose est un motif, un anti-motif ou un motif couramment mal appliqué qui peut encore être utilisé à certains endroits.

C'est juste faux.

Pour en ajouter un peu plus.

Ce code est superstitieux ou, au mieux, une pratique culte du fret.

Une superstition est une chose faite sans justification claire. Cela peut être lié à quelque chose de réel, mais la connexion n’est pas logique.

Une pratique de culte de fret est une pratique dans laquelle vous essayez de copier quelque chose que vous avez appris d’une source mieux informée, mais vous copiez en réalité les artefacts de surface plutôt que le processus (ainsi nommé pour un culte en Papouasie-Nouvelle-Guinée qui ferait des radios de contrôle des avions en bambou dans l’espoir de faire revenir les avions japonais et américains de la Seconde Guerre mondiale).

Dans les deux cas, il n'y a pas de véritable cas à justifier.

Un anti-modèle est une tentative d'amélioration raisonnable, que ce soit dans le petit (cette branche supplémentaire pour traiter le cas supplémentaire qui doit être traité, qui mène au code spaghetti) ou dans le grand où vous implémentez très délibérément un modèle soit discrédités, soit débattus (beaucoup décriraient les singletons en tant que tels, certains excluant les objets en écriture seule - par exemple, les objets de journalisation ou en lecture seule, par exemple les objets de paramètres de configuration - et certains condamneraient même ceux-ci), ou encore lorsque vous résolvez le problème. Mauvais problème (lorsque .NET a été mis en marché pour la première fois, MS a recommandé un modèle pour traiter l’élimination lorsque vous disposiez à la fois de champs non gérés et de champs gérés jetables - il résout effectivement très bien cette situation, mais le vrai problème est que vous avez les deux types de champs dans la même classe).

En tant que tel, un anti-modèle est quelque chose qu'une personne intelligente qui connaît bien la langue, le domaine problématique et les bibliothèques disponibles fera délibérément, qui a toujours (ou prétend avoir) un inconvénient qui le dépasse.

Aucun d'entre nous ne connaissant bien une langue donnée, un domaine problématique et les bibliothèques disponibles, et étant donné que tout le monde peut rater quelque chose lorsqu'il passe d'une solution raisonnable à une autre (par exemple, commencer à stocker quelque chose dans un champ pour un bon usage, puis essayer de refactorisez tout, mais ne terminez pas le travail, et vous obtiendrez du code comme dans la question), et comme nous manquons tous de temps en temps de choses dans l'apprentissage, nous avons tous créé un code superstitieux ou culte le cargo à un moment donné . La bonne chose est qu’ils sont en fait plus faciles à identifier et à corriger que les anti-modèles. Les véritables anti-patterns ne sont sans doute pas des anti-patterns, ou ont une qualité attrayante, ou ont au moins un moyen de les attirer, même lorsqu'ils sont identifiés comme mauvais (trop et trop peu de couches sont à la fois mauvais, mais en évitant un mène à l'autre).

Jon Hanna
la source
1
Mais les gens ne pensent que cela est une bonne idée, probablement parce qu'ils pensent que c'est une forme de réutilisation de code.
JSB ձոգչ
Les débutants semblent souvent penser que déclarer deux variables nécessite deux fois plus d'espace, et préfèrent donc réutiliser les variables. Cela montre une incompréhension du modèle de mémoire (magasin libre contre pile, possibilité de réutiliser des emplacements de pile) et des optimisations que tous les compilateurs sont capables d’exécuter. Selon moi, un débutant pourrait donc considérer la réutilisation des variables comme une bonne idée et le classer comme anti-modèle.
Philipp
Cela en fait une superstition, pas un anti-modèle.
Jon Hanna
3

(1) Je dirais que ce n'est pas bon.

Il s’agit de tâches manuelles que la pile peut effectuer automatiquement avec des variables locales.

Il n'y a aucun avantage en termes de performances puisque "nouveau" est appelé chaque invocation de méthode.

EDIT: L’empreinte mémoire de la classe est plus grande car le pointeur de la barre occupe toujours de la mémoire pendant la durée de vie de la classe. Si le pointeur sur la barre était local, la mémoire ne serait utilisée que pendant la durée de vie de l'appel de la méthode. Le souvenir du pointeur n'est qu'une goutte d'eau dans l'océan, mais c'est quand même une goutte inutile.

La barre est visible pour les autres méthodes de la classe. Les méthodes qui n'utilisent pas bar ne devraient pas pouvoir le voir.

Erreurs basées sur les états. Dans ce cas particulier, les erreurs de base d'état ne doivent se produire que dans le multi-threading car "new" est appelé à chaque fois.

(2) Je ne sais pas s’il ya un nom car c’est en fait plusieurs problèmes, pas un.
- entretien ménager manuel lorsque la fonction automatique est disponible
- état
non nécessaire - État qui dure plus longtemps que sa durée de vie -
visibilité ou portée trop élevée

mike30
la source
2

Je l'appellerais le "Xénophobe errant". Il veut rester seul mais finit par ne pas savoir exactement où et ce que c'est. C'est donc une mauvaise chose, comme d'autres l'ont déclaré.

Ingénieur du monde
la source
2

Aller à contre-courant ici mais bien que je ne considère pas l’exemple donné comme un bon style de codage tel qu’il est dans sa forme actuelle, le modèle existe lors des tests unitaires.

Cette manière assez standard de faire des tests unitaires

public class BarTester
{
    private Bar bar;

    public Setup() { bar = new Bar(); }
    public Teardown() { bar = null; }

    public TestMethodA()
    {
        bar.A();        
    }

    public TestMethodB()
    {
        bar.B();
    }
}

est juste une refactorisation de cet équivalent du code de OP

public class BarTester
{
    private Bar bar;

    public TestMethodA()
    {
        bar = new Bar();
        bar.A();
        bar = null;
    }

    public TestMethodB()
    {
        bar = new Bar();
        bar.B();
        bar = null;
    }
}

Je n'écrirais jamais le code tel que donné par l'exemple d'OP, cela appelle un refactoring, mais je considère que le modèle n'est ni invalide ni anti- modèle .

Lieven Keersmaekers
la source
Dans ce cas, la fixture est instanciée indépendamment pour chaque test et les problèmes liés à la réutilisation des variables ou à la simultanéité ne se produisent pas. Dans le cas général (tests unitaires extérieurs), on ne sait pas si au plus une méthode est appelée sur chaque objet.
Philipp
1
@Philipp - Je ne conteste pas le problème de la réutilisation ou de la concurrence, mais la question d'OP portait sur le modèle utilisé couramment dans les tests unitaires. Le fait que le montage soit instancié pour chaque test est un détail d'implémentation de la structure de test. Même dans les tests unitaires, le modèle n'est sécurisé que lorsque la structure de test est utilisée telle qu'elle est censée être utilisée. Le même raisonnement s'applique lors de l'utilisation de ce modèle dans le code de production.
Lieven Keersmaekers
Il n'est pas nécessaire de définir barsur null, JUnit crée de toute façon une nouvelle instance de test pour chaque méthode de test.
oberlies
2

Cette odeur de code est appelée champ temporaire dans la refactorisation par Beck / Fowler.

http://sourcemaking.com/refactoring/temporary-field

Modifié pour ajouter plus d'explications à la demande des modérateurs: Vous pouvez en savoir plus sur l'odeur de code dans l'URL référencée ci-dessus ou dans la copie papier du livre. Étant donné que le PO cherchait le nom de cette odeur particulière de code et anti-modèle, je pensais qu'il serait utile de citer une référence jamais mentionnée à ce sujet provenant d'une source bien établie de plus de 10 ans, même si je réalise parfaitement que je suis en retard. jeu sur cette question, il est donc peu probable que la réponse soit votée ou acceptée.

Si ce n’est pas personnellement une promotion, je fais également référence à cette odeur de code dans mon prochain cours Pluralsight, intitulé Refactoring Fundamentals, qui devrait être publié en août 2013.

Pour ajouter de la valeur, voyons comment résoudre ce problème particulier. Il y a plusieurs options. Si le champ n'est vraiment utilisé que par une méthode, il peut être rétrogradé en une variable locale. Toutefois, si plusieurs méthodes utilisent le champ pour communiquer (au lieu de paramètres), il s’agit du cas classique décrit dans Refactoring et peut être corrigé en remplaçant le champ par des paramètres ou si les méthodes qui fonctionnent avec ce code sont proches. connexe, Extract Class peut être utilisé pour extraire les méthodes et le (s) champ (s) dans leur propre classe, dans laquelle le champ est toujours dans un état valide (éventuellement défini lors de la construction) et représente l’état de cette nouvelle classe. Si vous passez par la route des paramètres, mais qu'il y a trop de valeurs à transmettre, une autre option consiste à introduire un nouvel objet paramètre,

ssmith
la source
Il est important de noter que des champs temporaires sont souvent nécessaires pour un refactoring de classe d'extraction. Mais quelqu'un qui est au courant de cela ne vérifiera probablement pas le code dans cet état intermédiaire.
oberlies
1

Je dirais quand vous y arrivez, c'est vraiment un manque de cohésion et je pourrais lui donner le nom de "Fausse cohésion". J'emploierais ce terme parce que la classe semble cohésive en ce sens que ses méthodes semblent fonctionner sur un champ membre. Cependant, en réalité, ils ne le font pas, ce qui signifie que la cohésion apparente est en réalité fausse.

Quant à savoir si c'est mauvais ou pas, je dirais que c'est clairement le cas, et je pense que Stephen C fait un excellent travail pour expliquer pourquoi. Toutefois, qu’il s’agisse ou non d’appeler "anti-motif", je pense que cela pourrait être fait avec tout autre paradigme de codage avec une étiquette, car cela facilite généralement la communication.

Erik Dietrich
la source
1

Outre les problèmes déjà mentionnés, l'utilisation de cette approche dans un environnement sans nettoyage automatique (par exemple, C ++) entraînerait des fuites de mémoire, car les objets temporaires ne sont pas libérés après leur utilisation. (Bien que cela puisse sembler un peu exagéré, il y a des gens qui changeraient simplement "null" en "NULL" et seraient heureux que le code soit compilé.)

Peterph
la source
1

Cela me semble être une mauvaise application du motif Flyweight. J'ai malheureusement rencontré quelques développeurs qui doivent utiliser la même "chose" plusieurs fois avec des méthodes différentes et l'ont simplement extrait dans un champ privé, dans une tentative malavisée d'économiser de la mémoire ou d'améliorer les performances, ou une autre pseudo-optimisation étrange. Dans leur esprit, ils essaient de résoudre un problème similaire, car Flyweight ne vise qu’ils ne le font que dans une situation où il n’ya pas réellement de problème à résoudre.

Evicatos
la source
-1

Je l'ai souvent rencontré, généralement lorsque le code rafraîchissant avait été écrit par quelqu'un qui avait choisi VBA pour les nuls comme premier titre et ensuite écrit un système relativement volumineux dans la langue dans laquelle il était tombé par hasard.

Dire que c’est une très mauvaise pratique de programmation est correct, mais c’est peut-être simplement dû au fait que nous n’avons pas accès à des ressources Internet et examinées par des pairs, ce dont nous jouissons tous aujourd’hui, qui auraient pu guider le développeur initial sur la voie de la «sécurité».

Cependant, il ne mérite pas vraiment la balise "antipattern" - mais cela a été bien de voir les suggestions sur la façon dont le PO pourrait être recodé.

Lee James
la source
1
Bienvenue aux programmeurs Stack Exchange, merci d’avoir entré votre première réponse. Il semble que vous ayez eu un vote négatif, peut-être pour l'une des raisons énumérées dans la FAQ programmers.stackexchange.com/faq . La FAQ donne d'excellentes suggestions pour apporter des réponses efficaces (et un vote plus élevé). Parfois, les gens ont la gentillesse d’ajouter une note sur le vote négatif afin d’indiquer s’il s’agit d’une erreur, d’une duplication, d’une opinion sans preuve, ou encore d’un mot qui me répète une réponse antérieure. La plupart d'entre nous ont eu un vote négatif, fermé ou supprimé des réponses, alors ne vous inquiétez pas. Cela fait partie du processus de démarrage. Bienvenue.
DeveloperDon