Quand les énumérations ne sont-elles PAS une odeur de code?

17

Dilemme

J'ai lu beaucoup de livres de bonnes pratiques sur les pratiques orientées objet, et presque tous les livres que j'ai lus avaient une partie où ils disent que les énumérations sont une odeur de code. Je pense qu'ils ont raté la partie où ils expliquent quand les énumérations sont valides.

En tant que tel, je recherche des directives et / ou des cas d'utilisation où les énumérations ne sont PAS une odeur de code et en fait une construction valide.

Sources:

"AVERTISSEMENT En règle générale, les énumérations sont des odeurs de code et doivent être refactorisées en classes polymorphes. [8]" Seemann, Mark, Dependency Injection in .Net, 2011, p. 342

[8] Martin Fowler et al., Refactoring: Improving the Design of Existing Code (New York: Addison-Wesley, 1999), 82.

Le contexte

La cause de mon dilemme est une API de trading. Ils me donnent un flux de données Tick en envoyant via cette méthode:

void TickPrice(TickType tickType, double value)

enum TickType { BuyPrice, BuyQuantity, LastPrice, LastQuantity, ... }

J'ai essayé de créer un wrapper autour de cette API, car la rupture des changements est le mode de vie de cette API. Je voulais garder une trace de la valeur de chaque dernier type de tick reçu sur mon wrapper et je l'ai fait en utilisant un Dictionary of ticktypes:

Dictionary<TickType,double> LastValues

Pour moi, cela semblait être une bonne utilisation d'une énumération si elles étaient utilisées comme clés. Mais j'ai des doutes parce que j'ai un endroit où je prends une décision basée sur cette collection et je ne peux pas penser à un moyen d'éliminer la déclaration de changement, je pourrais utiliser une usine mais cette usine aura toujours un basculer la déclaration quelque part. Il me semblait que je ne fais que bouger mais ça sent encore.

Il est facile de trouver les NE PAS des énumérations, mais les DO, pas si faciles, et j'apprécierais que les gens puissent partager leur expertise, les avantages et les inconvénients.

Doutes

Certaines décisions et actions sont basées sur celles TickType- ci et je n'arrive pas à penser à un moyen d'éliminer les instructions enum / switch. La solution la plus propre à laquelle je peux penser est d'utiliser une usine et de renvoyer une implémentation basée sur TickType. Même alors, j'aurai toujours une instruction switch qui renvoie une implémentation d'une interface.

La liste ci-dessous est l'un des exemples de classes où j'ai des doutes quant à l'utilisation incorrecte d'une énumération:

public class ExecutionSimulator
{
  Dictionary<TickType, double> LastReceived;
  void ProcessTick(TickType tickType, double value)
  {
    //Store Last Received TickType value
    LastReceived[tickType] = value;

    //Perform Order matching only on specific TickTypes
    switch(tickType)
    {
      case BidPrice:
      case BidSize:
        MatchSellOrders();
        break;
      case AskPrice:
      case AskSize:
        MatchBuyOrders();
        break;
    }       
  }
}
stromms
la source
26
Je n'ai jamais entendu parler d'énumérations comme une odeur de code. Pourriez-vous inclure une référence? Je pense qu'ils ont un sens énorme pour un nombre limité de valeurs potentielles
Richard Tingle
25
Quels livres disent que les énumérations sont une odeur de code? Obtenez de meilleurs livres.
JacquesB
3
La réponse à la question serait donc "la plupart du temps".
gnasher729
5
Une belle valeur sûre de type qui donne du sens? Cela garantit que les clés appropriées sont utilisées dans un dictionnaire? Quand est-ce une odeur de code?
7
Sans contexte, je pense qu'une meilleure formulation seraitenums as switch statements might be a code smell ...
suppliez

Réponses:

31

Les énumérations sont destinées aux cas d'utilisation lorsque vous avez littéralement énuméré toutes les valeurs possibles qu'une variable pourrait prendre. Déjà. Pensez à des cas d'utilisation comme les jours de la semaine ou les mois de l'année ou les valeurs de configuration d'un registre matériel. Des choses à la fois très stables et représentables par une simple valeur.

Gardez à l'esprit que si vous créez une couche anti-corruption, vous ne pouvez pas éviter d'avoir une instruction switch quelque part , en raison de la conception que vous enveloppez, mais si vous le faites correctement, vous pouvez la limiter à cet endroit et utiliser le polymorphisme ailleurs.

Karl Bielefeldt
la source
3
C'est le point le plus important - ajouter une valeur supplémentaire à une énumération signifie trouver chaque utilisation du type dans votre code et potentiellement avoir besoin d'ajouter une nouvelle branche pour la nouvelle valeur. Comparez avec un type polymorphe, où ajouter une nouvelle possibilité signifie simplement créer une nouvelle classe qui implémente l'interface - les changements sont concentrés en un seul endroit, donc plus faciles à faire. Si vous êtes sûr qu'un nouveau type de tick ne sera jamais ajouté, l'énumération est correcte, sinon il devrait être enveloppé dans une interface polymorphe.
Jules
C'est l'une des réponses que je cherchais. Je ne peux tout simplement pas l'éviter lorsque la méthode que j'encapsule utilise une énumération et le but est de centraliser ces énumérations en un seul endroit. Je dois faire le wrapper de telle sorte que si jamais l'API modifie ces énumérations, je n'ai qu'à changer le wrapper, et non les consommateurs du wrapper.
stromms
@Jules - "Si vous êtes sûr qu'un nouveau type de tick ne sera jamais ajouté ..." Cette affirmation est fausse à tant de niveaux. Une classe pour chaque type, malgré le fait que chaque type ait exactement le même comportement, est à peu près aussi loin d'une approche "raisonnable" que l'on pourrait éventuellement avoir. Ce n'est que lorsque vous avez des comportements différents et commencez à avoir besoin d'instructions "switch / if-then" que la ligne doit être tracée. Il n'est certainement pas basé sur le fait que je puisse ou non ajouter plus de valeurs d'énumération à l'avenir.
Dunk
@dunk Je pense que vous avez mal compris mon commentaire. Je n'ai jamais suggéré de créer plusieurs types avec un comportement identique - ce serait fou.
Jules
1
Même si vous avez "énuméré toutes les valeurs possibles", cela ne signifie pas que le type n'aura jamais de fonctionnalité supplémentaire par instance. Même quelque chose comme Month peut avoir d'autres propriétés utiles, comme le nombre de jours. L'utilisation d'une énumération empêche immédiatement l'extension du concept que vous modélisez. Il s'agit essentiellement d'un mécanisme / schéma anti-POO.
Dave Cousineau
17

Premièrement, une odeur de code ne signifie pas que quelque chose ne va pas. Cela signifie que quelque chose ne va pas. enumsent parce qu'il est souvent maltraité, mais cela ne signifie pas que vous devez les éviter. Il vous suffit de taper enum, d'arrêter et de rechercher de meilleures solutions.

Le cas particulier qui se produit le plus souvent est lorsque les différentes énumérations correspondent à différents types avec des comportements différents mais la même interface. Par exemple, parler à différents backends, rendre différentes pages, etc. Celles-ci sont beaucoup plus naturellement implémentées à l'aide de classes polymorphes.

Dans votre cas, le TickType ne correspond pas à des comportements différents. Ce sont différents types d'événements ou différentes propriétés de l'état actuel. Je pense donc que c'est l'endroit idéal pour une énumération.

Winston Ewert
la source
Voulez-vous dire que lorsque les énumérations contiennent des noms en tant qu'entrées (par exemple, des couleurs), elles sont probablement utilisées correctement. Et quand ce sont des verbes (Connecté, Rendu), c'est un signe qu'il est mal utilisé?
stromms
2
@stromms, la grammaire est un terrible guide de l'architecture logicielle. Si TickType était une interface au lieu d'une énumération, quelles seraient les méthodes? Je ne peux pas en voir, c'est pourquoi cela semble être un cas énuméré pour moi. Dans d'autres cas, comme le statut, les couleurs, le backend, etc., je peux trouver des méthodes, et c'est pourquoi ce sont des interfaces.
Winston Ewert
@WinstonEwert et avec l'édition, il semble que ce sont des comportements différents.
Ohhh. Donc, si je peux penser à une méthode pour une énumération, alors ce pourrait être un candidat pour une interface? Ce pourrait être un très bon point.
stromms
1
@stromms, pouvez-vous partager plus d'exemples de commutateurs, afin que je puisse avoir une meilleure idée de la façon dont vous utilisez TickType?
Winston Ewert du
8

Lors de la transmission des énumérations de données, aucune odeur de code

À mon humble avis, lors de la transmission de données en utilisant enumspour indiquer qu'un champ peut avoir une valeur à partir d'un ensemble restreint (changeant rarement) de valeurs est bon.

Je considère préférable de transmettre arbitrairement stringsou ints. Les chaînes peuvent provoquer des problèmes par des variations d'orthographe et de capitalisation. Ints permettre la transmission de valeurs de la plage et ont peu sémantique (par exemple , je reçois 3de votre service commercial, qu'est-ce que cela signifie? LastPrice? LastQuantity? Autre chose?

La transmission d'objets et l'utilisation de hiérarchies de classes n'est pas toujours possible; par exemple, ne permet pas à l'extrémité réceptrice de distinguer la classe qui a été envoyée.

Dans mon projet, le service utilise une hiérarchie de classes pour les effets des opérations, juste avant de transmettre via un DataContractobjet, la hiérarchie des classes est copiée dans un unionobjet similaire qui contient un enumpour indiquer le type. Le client reçoit le DataContractet crée un objet d'une classe dans une hiérarchie à l'aide de la enumvaleur de switchet crée un objet du type correct.

Une autre raison pour laquelle on ne voudrait pas transmettre des objets de classe est que le service peut nécessiter un comportement complètement différent pour un objet transmis (tel que LastPrice) que le client. Dans ce cas, l'envoi de la classe et de ses méthodes n'est pas souhaité.

Les instructions de commutation sont-elles mauvaises?

À mon humble avis , une déclaration unique switch qui appelle différents constructeurs en fonction d'un enumn'est pas une odeur de code. Elle n'est pas nécessairement meilleure ou pire que d'autres méthodes, telles que la réflexion basée sur un nom de type; cela dépend de la situation réelle.

Avoir des interrupteurs enumpartout est une odeur de code, fournit des alternatives souvent meilleures:

  • Utilisez un objet de différentes classes d'une hiérarchie qui ont des méthodes remplacées; c'est-à-dire polymorphisme.
  • Utilisez le modèle de visiteur lorsque les classes de la hiérarchie changent rarement et lorsque les (nombreuses) opérations doivent être couplées de manière lâche aux classes de la hiérarchie.
Kasper van den Berg
la source
En fait, TickType est transmis à travers le fil comme un int. Mon wrapper est celui qui utilise TickTypequi est casté à partir du reçu int. Plusieurs événements utilisent ce type de tick avec des signatures variant de manière sauvage qui sont des réponses à diverses demandes. Est-ce une pratique courante d'avoir une utilisation intcontinue pour différentes fonctions? par exemple, TickPrice(int type, double value)utilise 1,3 et 6 pour typetandis que TickSize(int type, double valueutilise 2,4 et 5? Est-il même logique de les séparer en deux événements?
stromms du
2

Et si vous optiez pour un type plus complexe:

    abstract class TickType
    {
      public abstract string Name {get;}
      public abstract double TickValue {get;}
    }

    class BuyPrice : TickType
    {
      public override string Name { get { return "Buy Price"; } }
      public override double TickValue { get { return 2.35d; } }
    }

    class BuyQuantity : TickType
    {
      public override string Name { get { return "Buy Quantity"; } }
      public override double TickValue { get { return 4.55d; } }
    }
//etcetera

alors vous pouvez charger vos types à partir de la réflexion ou le construire vous-même, mais la principale chose à faire ici est que vous maintenez le principe Open Close de SOLID

Chris
la source
1

TL; DR

Pour répondre à la question, j'ai maintenant du mal à penser à un moment où les énumérations ne sont pas une odeur de code à un certain niveau. Il y a une certaine intention qu'ils déclarent efficacement (il y a un nombre clairement limité et limité de possibilités pour cette valeur), mais leur nature intrinsèquement fermée les rend architecturalement inférieurs.

Excusez-moi alors que je refaçonne des tonnes de mon ancien code. / soupir; ^ D


Mais pourquoi?

Très bonne critique pourquoi c'est le cas de LosTechies ici :

// calculate the service fee
public double CalculateServiceFeeUsingEnum(Account acct)
{
    double totalFee = 0;
    foreach (var service in acct.ServiceEnums) { 

        switch (service)
        {
            case ServiceTypeEnum.ServiceA:
                totalFee += acct.NumOfUsers * 5;
                break;
            case ServiceTypeEnum.ServiceB:
                totalFee += 10;
                break;
        }
    }
    return totalFee;
} 

Cela a tous les mêmes problèmes que le code ci-dessus. À mesure que l'application s'agrandit, les chances d'avoir des instructions de branche similaires vont augmenter. De plus, au fur et à mesure que vous déployez des services premium, vous devrez continuellement modifier ce code, ce qui viole le principe ouvert-fermé. Il y a aussi d'autres problèmes ici. La fonction de calcul des frais de service ne devrait pas avoir besoin de connaître les montants réels de chaque service. Ce sont des informations qui doivent être encapsulées.

Un petit côté: les énumérations sont une structure de données très limitée. Si vous n'utilisez pas une énumération pour ce qu'elle est réellement, un entier étiqueté, vous avez besoin d'une classe pour modéliser correctement l'abstraction correctement. Vous pouvez utiliser la classe d'énumération impressionnante de Jimmy pour utiliser des classes pour les utiliser également comme étiquettes.

Refactorisons ceci pour utiliser un comportement polymorphe. Ce dont nous avons besoin, c'est d'une abstraction qui nous permettra de contenir le comportement nécessaire pour calculer les frais pour un service.

public interface ICalculateServiceFee
{
    double CalculateServiceFee(Account acct);
}

...

Nous pouvons maintenant créer nos implémentations concrètes de l'interface et les attacher [au] compte.

public class Account{
    public int NumOfUsers{get;set;}
    public ICalculateServiceFee[] Services { get; set; }
} 

public class ServiceA : ICalculateServiceFee
{
    double feePerUser = 5; 

    public double CalculateServiceFee(Account acct)
    {
        return acct.NumOfUsers * feePerUser;
    }
} 

public class ServiceB : ICalculateServiceFee
{
    double serviceFee = 10;
    public double CalculateServiceFee(Account acct)
    {
        return serviceFee;
    }
} 

Un autre cas d'implémentation ...

En fin de compte, si vous avez un comportement qui dépend des valeurs d'énumération, pourquoi ne pas avoir à la place différentes implémentations d'une interface ou d'une classe parent similaire qui garantit que cette valeur existe? Dans mon cas, je regarde différents messages d'erreur basés sur des codes d'état REST. Au lieu de...

private static string _getErrorCKey(int statusCode)
{
    string ret;

    switch (statusCode)
    {
        case StatusCodes.Status403Forbidden:
            ret = "BRANCH_UNAUTHORIZED";
            break;

        case StatusCodes.Status422UnprocessableEntity:
            ret = "BRANCH_NOT_FOUND";
            break;

        default:
            ret = "BRANCH_GENERIC_ERROR";
            break;
    }

    return ret;
}

... je devrais peut-être envelopper les codes de statut dans les classes.

public interface IAmStatusResult
{
    int StatusResult { get; }    // Pretend an int's okay for now.
    string ErrorKey { get; }
}

Ensuite, chaque fois que j'ai besoin d'un nouveau type d'IAmStatusResult, je le code ...

public class UnauthorizedBranchStatusResult : IAmStatusResult
{
    public int StatusResult => 403;
    public string ErrorKey => "BRANCH_UNAUTHORIZED";
}

... et maintenant je peux m'assurer que le code antérieur se rend compte qu'il a une IAmStatusResultportée et référence son entity.ErrorKeyau lieu du cul-de-sac le plus compliqué _getErrorCode(403).

Et, plus important encore, chaque fois que j'ajoute un nouveau type de valeur de retour, aucun autre code n'a besoin d'être ajouté pour le gérer . Whaddya sait, enumet switchétaient probablement des odeurs de code.

Profit.

ruffin
la source
Qu'en est-il du cas où, par exemple, j'ai des classes polymorphes et que je souhaite configurer celle que j'utilise via un paramètre de ligne de commande? Ligne de commande -> enum -> switch / map -> l'implémentation semble un cas d'utilisation assez légitime. Je pense que l'élément clé est que l'énumération est utilisée pour l'orchestration, pas comme une implémentation de la logique conditionnelle.
Ant P
@AntP Pourquoi ne pas, dans ce cas, utiliser la StatusResultvaleur? Vous pourriez faire valoir que l'énumération est utile en tant que raccourci mémorisable dans ce cas d'utilisation, mais j'appellerais probablement encore cela une odeur de code, car il existe de bonnes alternatives qui ne nécessitent pas la collection fermée.
ruffin
Quelles alternatives? Un string? C'est une distinction arbitraire du point de vue des «énumérations devraient être remplacées par du polymorphisme» - l'une ou l'autre approche nécessite de mapper une valeur à un type; éviter l'énumération dans ce cas n'aboutit à rien.
Ant P
@AntP Je ne sais pas où les fils se croisent ici. Si vous référencez une propriété sur une interface, vous pouvez jeter cette interface partout où cela est nécessaire et ne jamais mettre à jour ce code lorsque vous créez une nouvelle implémentation (ou supprimez une implémentation existante) de cette interface . Si vous avez un enum, vous ne devez le code de mise à jour chaque fois que vous ajoutez ou supprimez une valeur, et potentiellement beaucoup d'endroits, en fonction de la façon dont vous structuré votre code. Utiliser le polymorphisme au lieu d'une énumération revient en quelque sorte à s'assurer que votre code est au format normal Boyce-Codd .
ruffin
Oui. Supposons maintenant que vous ayez N de telles implémentations polymorphes. Dans l'article Los Techies, ils sont simplement en train de répéter à travers chacun d'eux. Mais que se passe-t-il si je souhaite appliquer conditionnellement une seule implémentation basée, par exemple, sur des paramètres de ligne de commande? Maintenant, nous devons définir un mappage d'une valeur de configuration à un type d'implémentation injectable. Dans ce cas, une énumération est aussi bonne que tout autre type de configuration. Ceci est un exemple d'une situation où il n'y a plus de possibilité de remplacer le "sale enum" par le polymorphisme. La branche par abstraction ne peut que vous mener jusqu'ici.
Ant P
0

Que l'utilisation d'une énumération soit ou non une odeur de code dépend du contexte. Je pense que vous pouvez obtenir quelques idées pour répondre à votre question si vous considérez le problème d'expression . Donc, vous avez une collection de différents types et une collection d'opérations sur eux, et vous devez organiser votre code. Il existe deux options simples:

  • Organisez le code selon les opérations. Dans ce cas, vous pouvez utiliser une énumération pour baliser différents types et avoir une instruction switch dans chaque procédure qui utilise les données balisées.
  • Organisez le code selon les types de données. Dans ce cas, vous pouvez remplacer l'énumération par une interface et utiliser une classe pour chaque élément de l'énumération. Vous implémentez ensuite chaque opération en tant que méthode dans chaque classe.

Quelle solution est la meilleure?

Si, comme l'a souligné Karl Bielefeldt, vos types sont fixes et que vous vous attendez à ce que le système se développe principalement en ajoutant de nouvelles opérations sur ces types, alors utiliser une énumération et avoir une instruction switch est une meilleure solution: chaque fois que vous avez besoin d'une nouvelle opération, vous implémentez simplement une nouvelle procédure alors qu'en utilisant des classes, vous devrez ajouter une méthode à chaque classe.

D'un autre côté, si vous vous attendez à avoir un ensemble d'opérations plutôt stable mais que vous pensez que vous devrez ajouter plus de types de données au fil du temps, l'utilisation d'une solution orientée objet est plus pratique: comme de nouveaux types de données doivent être implémentés, vous venez continuez à ajouter de nouvelles classes implémentant la même interface, alors que si vous utilisez une énumération, vous devrez mettre à jour toutes les instructions switch dans toutes les procédures utilisant l'énumération.

Si vous ne pouvez pas classer votre problème dans l'une des deux options ci-dessus, vous pouvez rechercher des solutions plus sophistiquées (voir par exemple à nouveau la page Wikipedia citée ci-dessus pour une brève discussion et quelques références pour une lecture plus approfondie).

Vous devez donc essayer de comprendre dans quelle direction votre application peut évoluer, puis choisir une solution appropriée.

Étant donné que les livres auxquels vous vous référez traitent du paradigme orienté objet, il n'est pas surprenant qu'ils soient biaisés contre l'utilisation d'énumérations. Cependant, une solution d'orientation d'objet n'est pas toujours la meilleure option.

Conclusion: les énumérations ne sont pas nécessairement une odeur de code.

Giorgio
la source
notez que la question a été posée dans le contexte de C #, un langage POO. aussi, que le regroupement par opération ou par type sont les deux faces d'une même pièce est intéressant, mais je ne vois pas l'un comme étant "meilleur" que l'autre comme vous le décrivez. la quantité de code résultante est la même et les langages POO facilitent déjà l'organisation par type. il produit également un code beaucoup plus intuitif (facile à lire).
Dave Cousineau
@DaveCousineau: "Je ne vois pas l'un comme étant" meilleur "que l'autre comme vous le décrivez": je n'ai pas dit que l'un est toujours meilleur que l'autre. J'ai dit que l'un est meilleur que l'autre selon le contexte. Par exemple , si les opérations sont plus ou moins fixes et que vous prévoyez sur l' ajout de nouveaux types (par exemple une interface graphique avec fixe paint(), show(), close() resize()approche des opérations et des widgets personnalisés) , puis l'orienté objet est mieux dans le sens où il permet d'ajouter un nouveau type sans affecter trop beaucoup de code existant (vous implémentez essentiellement une nouvelle classe, qui est un changement local).
Giorgio
Je ne pense pas non plus que ce soit "mieux" comme vous l'avez décrit, ce qui signifie que je ne vois pas que cela dépend de la situation. La quantité de code que vous devez écrire est exactement la même dans les deux scénarios. La seule différence est qu'une façon est contraire à la POO (énumérations) et une ne l'est pas.
Dave Cousineau
@DaveCousineau: La quantité de code que vous devez écrire est probablement la même, la localité (endroits dans le code source que vous devez modifier et recompiler) change radicalement. Par exemple, dans la POO, si vous ajoutez une nouvelle méthode à une interface, vous devez ajouter une implémentation pour chaque classe qui implémente cette interface.
Giorgio
N'est-ce pas simplement une question de largeur VS de profondeur? Ajouter 1 méthode à 10 fichiers ou 10 méthodes à 1 fichier.
Dave Cousineau