Est-ce une odeur de code si vous créez fréquemment un objet juste pour appeler une méthode dessus

14

J'ai hérité d'une base de code où il y a beaucoup de code qui ressemble à ceci:

SomeDataAdapter sda = new SomeDataAdapter();
sda.UpdateData(DataTable updateData);

Et puis sda n'est plus jamais utilisé.

Est-ce une odeur de code qui indique que ces méthodes devraient en fait être des méthodes de classe statiques?

Shane Wealti
la source
Ces classes implémentent-elles des interfaces?
Reactgular
7
Le jour après avoir refactorisé les méthodes statiques sera le jour où vous souhaitez exécuter des tests unitaires avec une version simulée de votre adaptateur de données.
Mike
22
@Mike: Pourquoi auriez-vous besoin de vous moquer d'une méthode statique? Je suis terriblement fatigué de cet argument fallacieux selon lequel les méthodes statiques ne sont pas testables. Si vos méthodes statiques maintiennent l'état ou créent des effets secondaires, c'est votre faute, pas la faute de votre méthodologie de test.
Robert Harvey
9
À mon avis, c'est une odeur de langage montrant la faiblesse de l'analyse "tout doit être une classe".
Ben
7
@RobertHarvey: vous devrez peut-être simuler une méthode statique pour la même raison que vous simulez toute autre méthode: il est trop coûteux d'appeler pendant les tests unitaires.
kevin cline

Réponses:

1

Je considérerais cela comme une odeur d'architecture dans la mesure où UpdateData devrait probablement appartenir à une classe de «service».

Où les données sont une pomme. Où AppleAdapter est une classe de service / intelligence d'affaires. Où AppleService est une référence Singleton à un AppleAdapter qui existe en dehors de la méthode actuelle.

private static volatile AppleAdapter _appleService = null;
private static object _appleServiceLock = new object();
private AppleAdapter AppleService
{
    get
    {
        if (_appleService == null)
        {
            lock (_appleServiceLock)
            {
                if (_appleService == null)
                    _appleService = new AppleAdapter();
            }
        }
        return _appleService;
    }
}

public SomeAppleRelatedMethod(Apple apple)
{
    AppleService.UpdateData(apple);
}

Je ne pense pas que ce que vous faites soit nécessairement faux, mais si SomeDataAdapter représente effectivement une sorte de service commercial sans état, alors un singleton serait la meilleure pratique pour cela. J'espère que cela pourra aider! L'exemple fourni est un moyen sophistiqué de garantir qu'aucune contention de _appleService ne se trouve être à la fois nulle et accessible exactement en même temps par deux threads ou plus.

Vous savez quoi? Si SomeDataAdapter est un ADO IDbDataAdapter (ce qu'il est presque certainement), ignorez cette réponse entière!

: P

Je n'ai pas la permission d'ajouter un commentaire à la question d'origine, mais si vous pouviez spécifier où ce code existe.

Si ce code représente une implémentation personnalisée d'un IDbDataAdapter, et UpdateData crée un IDbConnection, IDbCommand, et le câblage tout en arrière-plan, alors je ne considérerais pas qu'une odeur de code parce que maintenant nous parlons de flux et autres les choses qui doivent être éliminées lorsque nous avons fini de les utiliser.

Andrew Hoffman
la source
12
Aidez-moi! Je me noie dans les modèles logiciels.
Robert Harvey
4
... ou injecté comme dépendance. ;)
Rob
12
Jouer avec des singletons et un verrouillage à double vérification pour obtenir quelque chose d'équivalent à une simple fonction / procédure est une odeur de code beaucoup plus grande pour moi!
Ben
3

Je pense que ce problème va encore plus loin que cela. Même si vous le refactorisez en une méthode statique, vous obtenez du code où vous appelez une méthode statique unique partout. Ce qui, à mon avis, est une odeur de code en soi. Cela pourrait indiquer que vous manquez une abstraction importante. Peut-être que vous voulez créer un code qui fera un certain pré et post-traitement tout en vous permettant de changer ce qui se passe entre les deux. Ce pré et post-traitement contiendra les appels communs tandis que l'intervalle dépendra de la mise en œuvre concrète.

Euphorique
la source
Je suis d'accord qu'il y a un problème beaucoup plus profond. J'essaie de l'améliorer progressivement sans une refonte architecturale complète, mais il semble que ce que je pensais n'est pas un bon moyen de le faire.
Shane Wealti
1

En quelque sorte. Cela signifie généralement que vous souhaitez transmettre une fonction et que la langue de votre choix ne vous le permettra pas. C'est un peu maladroit et inélégant, mais si vous êtes coincé dans un langage sans fonctions de première classe, vous ne pouvez pas faire grand-chose.

Si aucun de ces objets n'est transmis comme arguments à d'autres fonctions, vous pouvez les transformer en méthodes statiques et rien ne changera.

Doval
la source
Cela peut également signifier que vous souhaitez que la méthode statique renvoie une version modifiée (ou une copie modifiée) d'un objet que vous lui passez.
Robert Harvey
2
"si vous êtes coincé dans un langage sans fonctions de première classe": Il n'y a pas de différence entre un objet avec une seule méthode et une fonction de première classe (ou fermeture de première classe): ce ne sont que deux vues différentes de la même chose.
Giorgio
4
@Giorgio Théoriquement, non. En pratique, si votre langue n'a pas au moins de sucre de syntaxe, c'est la différence entre fn x => x + 1et new Function<Integer, Integer>() { public Integer eval(Integer x) { return x + 1; }};ou vous limiter à quelques fonctions codées en dur et étroitement utiles.
Doval
Pourquoi ne pourrait-on pas fn x => x + 1utiliser du sucre syntaxique new Function<Integer, Integer>() { public Integer eval(Integer x) { return x + 1; }}? Ok, peut-être voulez-vous dire si l'on est coincé dans une langue sans ce type de sucre syntaxique.
Giorgio
@Giorgio Vous devrez demander à Oracle. (Certes, cela change finalement avec Java 8). Notez que vous auriez besoin de plus de sucre de syntaxe que cela pour être vraiment utile - vous voudriez également pouvoir passer des méthodes pré-déclarées par nom. Le curry serait également pratique. À un moment donné, vous devez vous demander s'il vaut la peine d'avoir tous ces hacks au lieu de traiter les fonctions comme des premiers citoyens. Mais maintenant, je m'éloigne du sujet.
Doval
0

Si l'état de détention rend une classe donnée plus utilitaire, fonctionnelle… réutilisable, alors non. Pour une raison quelconque, le modèle de conception de commande vient à l'esprit; cette raison n'est certainement pas le désir de faire noyer Robert Harvey.

radarbob
la source
0

Définissez «fréquemment». Combien de fois instanciez-vous l'objet? Beaucoup - probablement pas?

Il y a probablement de plus gros problèmes à craindre dans votre système. Le fait de devenir statique communiquera plus clairement au programmeur que l'état interne de l'objet ne change pas - parfait.

Si l'état est gâché, et que vous devez commencer à rendre d'autres choses statiques pour rendre la première chose statique, vous devez alors demander quelles parties doivent être statiques et quelles parties ne le doivent pas.

Oui, c'est une odeur de code, juste une petite.

user1775944
la source
0

Faites-en une méthode statique et continuez. Il n'y a rien de mal avec les méthodes statiques. Une fois qu'un modèle d'utilisation émerge, vous pouvez vous soucier de l'abstraire.

davidk01
la source
0

L'odeur ici est que c'est du code procédural enveloppé (de façon minimale) dans des objets.

Il doit y avoir une raison pour laquelle vous souhaitez effectuer cette opération sur la table de données, mais plutôt que de modéliser cette opération avec d'autres opérations connexes qui partagent certaines sémantiques (c'est-à-dire qui ont une signification commune), l'auteur vient de créer une nouvelle procédure à l'intérieur d'une nouvelle classe et l'a appelé.

Dans un langage fonctionnel, c'est comme ça que vous feriez les choses;) mais dans le paradigme OO, vous voudriez modéliser une abstraction qui incluait cette opération. Ensuite, vous utiliseriez des techniques OO pour étoffer ce modèle et fournir un ensemble d'opérations connexes qui préservent une certaine sémantique.

Donc, l'odeur principale ici est que l'auteur utilise des classes, probablement parce que le compilateur l'exige, sans organiser les opérations dans un modèle conceptuel.

BTW attention à chaque fois que vous voyez le programme en cours de modélisation au lieu de l' espace de problème . C'est un signe que le design pourrait bénéficier de plus de réflexion. En d'autres termes, faites attention aux classes qui sont toutes "xAdaptor" et "xHandler" et "xUtility", et généralement liées à un modèle de domaine anémique . Cela signifie que quelqu'un regroupe simplement du code pour plus de commodité, au lieu de modéliser réellement les concepts qu'il souhaite implémenter.

Rob
la source