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

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?

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.

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.

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é.

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.

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

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).

(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

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é.

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 .

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,

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.

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é.)

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.

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é.