15

Dilemma

I've been reading a lot of best practice books about object oriented practices, and almost every book I've read had a part where they say that enums are a code smell. I think they've missed the part where they explain when enums are valid.

As such, I am looking for guidelines and/or use-cases where enums are NOT a code smell and in fact a valid construct.

Sources:

"WARNING As a rule of thumb, enums are code smells and should be refactored to polymorphic classes. [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.

Context

The cause of my dilemma is a trading API. They give me a stream of Tick data by sending thru this method:

void TickPrice(TickType tickType, double value)

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

I've tried making a wrapper around this API because breaking changes is the way of life for this API. I wanted to keep track of the value of each last received tick type on my wrapper and I've done that by using a Dictionary of ticktypes:

Dictionary<TickType,double> LastValues

To me, this seemed like a proper use of an enum if they are used as keys. But I am having second thoughts because I do have a place where I make a decision based on this collection and I can't think of a way how I could eliminate the switch statement, I could use a factory but that factory will still have a switch statement somewhere. It seemed to me that I'm just moving things around but it still smells.

It's easy to find the DON'Ts of enums, but the DOs, not that easy, and I'd appreciate it if people can share their expertise, the pros and cons.

Second thoughts

Some decisions and actions are based on these TickType and I can't seem to think of a way to eliminate enum/switch statements. The cleanest solution I can think of is using a factory and return an implementation based on TickType. Even then I will still have a switch statement that returns an implementation of an interface.

Listed below is one of the sample classes where I'm having doubts that I might be using an enum wrong:

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;
    }       
  }
}
  • 25
    I've never heard of enums being a code smell. Could you include a reference? I think they make huge sense for a limited number of potential values – Richard Tingle Oct 16 '15 at 21:07
  • 23
    What books are saying enums are a code smell? Get better books. – JacquesB Oct 16 '15 at 21:08
  • 3
    So the answer to the question would be "most of the time". – gnasher729 Oct 16 '15 at 21:09
  • 5
    A nice, type safe value that conveys meaning? That guarantees the proper keys are used in a Dictionary? When is it a code smell? – user40980 Oct 16 '15 at 21:14
  • 7
    Without context I think a better wording would be enums as switch statements might be a code smell ... – pllee Oct 16 '15 at 23:04
28

Enums are intended for use cases when you have literally enumerated every possible value a variable could take. Ever. Think use cases like days of the week or months of the year or config values of a hardware register. Things that are both highly stable and representable by a simple value.

Keep in mind, if you're making an anti-corruption layer, you can't avoid having a switch statement somewhere, because of the design you're wrapping, but if you do it right you can limit it to that one place and use polymorphism elsewhere.

  • 2
    This is the most important point - adding an extra value to an enum means finding every use of the type in your code and potentially needing to add a new branch for the new value. Compare with a polymorphic type, where adding a new possibility simply means creating a new class that implements the interface - the changes are concentrated in a single place, so easier to make. If you're sure a new tick type will never be added, the enum is fine, otherwise it should be wrapped in a polymorphic interface. – Jules Oct 17 '15 at 3:34
  • That's one of the answers I was looking for. I just can't avoid it when the method I'm wrapping uses an enum and the aim is to centralize these enum in one place. I have to make the wrapper such that if ever the API changes these enums, I only have to change the wrapper, and not the consumers of the wrapper. – stromms Oct 18 '15 at 22:04
  • @Jules - "If you're sure a new tick type will never be added..." That statement is wrong on so many levels. A class for every single type, despite each type having the exact same behavior is about as far from a "reasonable" approach as one could possibly get. It isn't until you have different behaviors and start needing "switch/if-then" statements where the line needs to be drawn. It is certainly not based on whether or not I might add more enum values in the future. – Dunk Nov 17 '15 at 22:46
  • @dunk I think you misunderstood my comment. I never suggested created multiple types with identical behaviour - that would be crazy. – Jules Nov 19 '15 at 17:56
  • 1
    Even if you've "enumerated every possible value" that doesn't mean that the type will never have additional functionality per-instance. Even something like Month can have other useful properties, like number of days. Using an enum immediately prevents the concept you're modeling from being extended. It's basically an anti-OOP mechanism/pattern. – Dave Cousineau Oct 17 '18 at 22:24
17

Firstly, a code-smell doesn't mean that something is wrong. It means that something might be wrong. enum smells because its frequently abused, but that doesn't mean that you have to avoid them. Just you find yourself typing enum, stop and check for better solutions.

The particular case that happens most often is when the different enum's correspond to different types with different behaviors but the same interface. For example, talking to different backends, rendering different pages, etc. These are much more naturally implemented using polymorphic classes.

In your case, the TickType doesn't correspond to different behaviors. They are different types of events or different properties of the current state. So I think this is ideal place for an enum.

  • Are you saying that when enums contains Nouns as entries (e.g. Colors), they are probably used correctly. And when they are verbs (Connected, Rendering) then it's a sign that it's being misused? – stromms Oct 18 '15 at 21:53
  • 2
    @stromms, grammar is a terrible guide to software architecture. If TickType were an interface instead of an enum, what would the methods be? I can't see any, that's why it seems to be an enum case to me. Other cases, like status, colors, backend, etc, I can come up with methods, and that's why they are interfaces. – Winston Ewert Oct 19 '15 at 3:26
  • @WinstonEwert and with the edit, it appears that they are different behaviors. – user40980 Oct 20 '15 at 19:02
  • Ohhh. SO if I can think of a method for an enum, then it might be a candidate for an interface? That might be a very good point. – stromms Oct 20 '15 at 19:03
  • 1
    @stromms, can you share more examples of switches, so I can get a better idea how you use TickType? – Winston Ewert Oct 20 '15 at 21:21
6

When transmitting data enums are no code smell

IMHO, when transmitting data using enums to indicate that a field can have a value from a restricted (seldom changing) set of values is good.

I consider it preferable to transmitting arbitrary strings or ints. Strings may cause problems by variations in spelling and capitalisation. Ints allow for transmitting out of range values and have little semantics (e.g. I receive 3 from your trading service, what does it mean? LastPrice? LastQuantity? Something else?

Transmitting objects and using class hierarchies is not always possible; for example does not allow the receiving end to distinguish which class has been sent.

In my project the service uses a class hierarchy for effects of operations, just before transmitting via a DataContract the object from the class hierarchy is copied into a union-like object which contains an enum to indicate the type. The client receives the DataContract and creates an object of a class in a hierarchy using the enum value to switch and create an object of the correct type.

An other reason why one would not want to transmit objects of class is that the service may require completely different behaviour for a transmitted object (such as LastPrice) than the client. In that case sending the class and its methods is undesired.

Are switch statements bad?

IMHO, a single switch-statement that calls different constructors depending on an enum is not a code smell. It is not necessarily better or worse than other methods, such as reflection base on a typename; this depends on the actual situation.

Having switches on an enum all over the place is a code smell, provides alternatives that are often better:

  • Use object of different classes of a hierarchy that have overridden methods; i.e. polymorphism.
  • Use the visitor pattern when the classes in the hierarchy seldom change and when the (many) operations should be loosely coupled to the classes in the hierarchy.
  • Actually, TickType is being transmitted thru the wire as an int. My wrapper is the one that uses TickType which is casted from the received int. Several events use this ticktype with wild varying signatures that are responses to various requests. Is it common practice to have use an int continously for different functions? e.g.TickPrice(int type, double value) uses 1,3, and 6 for type while TickSize(int type, double value uses 2,4, and 5? Does it even make sense to separate those into two events? – stromms Oct 18 '15 at 22:30
2

what if you went with a more complex type:

    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

then you could load your types from reflection or build it yourself but the primary thing going here is that you are holding to Open Close Principle of SOLID

0

Whether using an enum is a code smell or not depends on the context. I think you can get some ideas for answering your question if you consider the expression problem. So, you have a collection of different types and a collection of operations on them, and you need to organize your code. There are two simple options:

  • Organize the code according to the operations. In this case you can use an enumeration to tag different types, and have a switch statement in each procedure that uses the tagged data.
  • Organize the code according to the data types. In this case you can replace the enumeration by an interface and use a class for each element of the enumeration. You then implement each operation as a method in each class.

Which solution is better?

If, as Karl Bielefeldt pointed out, your types are fixed and you expect the system to grow mainly by adding new operations on these types, then using an enum and having a switch statement is a better solution: each time you need a new operation you just implement a new procedure whereas by using classes you would have to add a method to each class.

On the other hand, if you expect to have a rather stable set of operation but you think you will have to add more data types over time, using an object-oriented solution is more convenient: as new data types must be implemented, you just keep adding new classes implementing the same interface whereas if you were using an enum you would have to update all switch statements in all procedures using the enum.

If you cannot classify your problem in either of the two options above, you can look at more sophisticated solutions (see e.g. again the Wikipedia page cited above for a short discussion and some reference for further reading).

So, you should try to understand in which direction your application may evolve, and then pick a suitable solution.

Since the books you refer to deal with the object-oriented paradigm, it is not surprising that they are biased against using enums. However, an object-orientation solution is not always the best option.

Bottomline: enums are not necessarily a code smell.

  • note that the question was asked in the context of C#, an OOP language. also, that grouping by operation or by type are two sides of the same coin is interesting, but I don't see one as being "better" than the other as you describe. the resulting amount of code is the same, and OOP languages already facilitate organizing by type. it also produces substantially more intuitive (easy to read) code. – Dave Cousineau Oct 17 '18 at 22:45
  • @DaveCousineau: "I don't see one as being "better" than the other as you describe": I did not say that one is always better than the other. I said that one is better than the other depending on the context. E.g. if operations are more or less fixed and you plan on adding new types (e.g. a GUI with fixed paint(), show(), close() resize() operations and custom widgets) then the object-oriented approach is better in the sense that it allows to add a new type without affecting too much existing code (you basically implement a new class, which is a local change). – Giorgio Oct 18 '18 at 17:37
  • I don't see either as being "better" as you described meaning that I don't see that it depends on the situation. The amount of code you have to write is exactly the same in both scenarios. The only difference is that one way is antithetical to OOP (enums) and one isn't. – Dave Cousineau Oct 18 '18 at 18:41
  • @DaveCousineau: The amount of code you have to write is probably the same, the locality (places in the source code that you have to change and recompile) changes drastically. E.g. in OOP, if you add a new method to an interface, you have to add an implementation for each class that implements that interface. – Giorgio Oct 18 '18 at 19:22
  • Isn't it just a matter of breadth VS depth? Adding 1 method to 10 files or 10 methods to 1 file. – Dave Cousineau Oct 18 '18 at 20:11
0

TL;DR

To answer the question, I'm now having a hard time thinking of a time enums are not a code smell on some level. There's a certain intent that they declare efficiently (there is a clearly bounded, limited number of possibilities for this value), but their inherently closed nature makes them architecturally inferior.

Excuse me as I refactor tons of my legacy code. /sigh ;^D


But... why?

Pretty good review why that's the case from LosTechies here:

// 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;
} 

This has all of the same problems as the code above. As the application gets bigger, the chances of having similar branch statements are going to increase. Also as you roll out more premium services you’ll have to continually modify this code, which violates the Open-Closed Principle. There are other problems here too. The function to calculate service fee should not need to know the actual amounts of each service. That is information that needs to be encapsulated.

A slight aside: enums are a very limited data structure. If you are not using an enum for what it really is, a labeled integer, you need a class to truly model the abstraction correctly. You can use Jimmy’s awesome Enumeration class to use classes to also us them as labels.

Let’s refactor this to use polymorphic behavior. What we need is abstraction that will allow us to contain the behavior necessary to calculate the fee for a service.

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

...

Now we can create our concrete implementations of the interface and attach them [to] the account.

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;
    }
} 

Another implementation case...

The bottom line is that if you have behavior that depends on enum values, why not instead have different implementations of a similar interface or parent class that ensures that value exists? In my case, I'm looking at different error messages based on REST status codes. Instead of...

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;
}

... perhaps I should wrap status codes in classes.

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

Then each time I need a new type of IAmStatusResult, I code it up...

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

... and now I can ensure that earlier code realizes it has an IAmStatusResult in scope and reference its entity.ErrorKey instead of the more convoluted, deadend _getErrorCode(403).

And, more importantly, every time I add a new type of return value, no other code needs to be added to handle it. Whaddya know, the enum and switch were likely code smells.

Profit.

  • How about the case where, e.g., I have polymorphic classes and I want to configure which one I'm using via a command line parameter? Command line -> enum -> switch/map -> implementation seems a pretty legitimate use case. I think the key thing is that the enum is used for orchestration, not as an implementation of conditional logic. – Ant P Jul 17 at 14:22
  • @AntP Why not, in this case, use the StatusResult value? You could argue that the enum is useful as a human-memorable shortcut in that use case, but I'd probably still call that a code smell, as there are good alternatives that don't require the closed collection. – ruffin Jul 17 at 19:58
  • What alternatives? A string? That's an arbitrary distinction from the perspective of "enums should be replaced with polymorphism" - either approach necessitates mapping a value to a type; avoiding the enum in this case achieves nothing. – Ant P Jul 17 at 23:14
  • @AntP I'm not sure where the wires are crossing here. If you reference a property on an interface, you can throw around that interface anywhere it's needed and never update that code when you create a new (or remove an existing) implementation of that interface. If you have an enum, you do have to update code every time you add or remove a value, and potentially lots of places, depending on how you structured your code. Using polymorphism instead of an enum is sorta like ensuring your code is in Boyce-Codd Normal Form. – ruffin Jul 18 at 20:17
  • Yes. Now suppose you have N such polymorphic implementations. In the Los Techies article, they are simply iterating through all of them. But what if I want to conditionally apply just one implementation based on e.g. command line parameters? Now we must define a mapping from some configuration value to some injectable implementation type. An enum in this case is as good as any other configuration type. This is an example of a situation where there is no further opportunity to replace the "dirty enum" with polymorphism. Branch by abstraction can only take you so far. – Ant P Jul 19 at 7:03

protected by gnat Jul 17 at 15:59

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.