When are enums NOT a code smell?

Question

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

Answer 1

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.

Answer 2

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.

Answer 3

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 wcf 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, oop 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.

Answer 4

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.

Answer 5

Enums are generally okay, they serve a meaningful purpose to represent a data type that can only take a limited number of concrete, possible values.

The two major problems with enums are:

1. They are often used in situations where polymorphism would make a lot more sense, would be easier to extend and would lead to more stable and easier to maintain code.

2. If you alter an enum once it has been released to public, all hell may break lose, especially in situations where code is shipped in compiled form.

As long as your enum is stable (you will never alter it again once released) or you may alter it, this fact is documented and you do it in a backward compatible way, and as long as you don't use it instead of polymorphism, I see nothing that would speak against them.

As a tip: Never store enums persistently.

If you need to store enum values to a file, map them other values and on load, map them back, e.g. map them to strings, store them as string, map strings back to enums when loading the data. That way you decouple the stored data from enum values. If enum values change, this will force you to recompile your code but at least the recompiled code will still correctly load the data which would not be the case if you had stored raw enum values. In the end you only need two simple mapping tables and the process is fast.

Answer 6

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

Answer 7

TL;DR -- enums are always a code smell.

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.

Instead of sending enums, send types that implement an interface that represents the same logic, with perhaps a method per normalized switch use case. Now you don't have to find and update switches whenever you add a new case/state. By calling the methods on the objects that've implemented the interface, you accomplish the same thing without losing DRYness.

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

---

How did you come to this conclusion?

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. [emphasis and link mine]

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

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 _getErrorCKey(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.

Step 3: Profit. Refactor.