37

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;
    }       
  }
}
stromms
  • 505
  • 1
  • 4
  • 6
  • 49
    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
  • 43
    What books are saying enums are a code smell? Get better books. – JacquesB Oct 16 '15 at 21:08
  • 6
    So the answer to the question would be "most of the time". – gnasher729 Oct 16 '15 at 21:09
  • 12
    A nice, type safe value that conveys meaning? That guarantees the proper keys are used in a Dictionary? When is it a code smell? –  Oct 16 '15 at 21:14
  • 1
    @JacquesB added my sources. I don't have a copy of Martin Fowler's book though so I don't have context. – stromms Oct 16 '15 at 22:34
  • 4
    Not everyone works on huge enterprises systems where every class has 20 layers of inheritance. – whatsisname Oct 16 '15 at 22:45
  • 1
    If you're using a value that you know is going to expand to more values than I'd say it's improperly being used – Chris Oct 16 '15 at 22:45
  • I also think that enums are commonly used as 'Value types' which are normally more complex – Chris Oct 16 '15 at 22:47
  • @whatsisname I don't think having 20 layers of inheritance is a good design. Prefer composition over inheritance is the guideline and I've stuck to this since I got into trouble with overly complex inheritance trees. – stromms Oct 16 '15 at 22:50
  • I dont believe the Refactoring book states that enums in general are a code smell. Using enums as "type codes" for objects is bad, but there are many legitimate uses of enums. – JacquesB Oct 16 '15 at 22:56
  • 9
    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
  • 2
    Tagged unions use enums and switch statements in a pre-object-oriented way of doing similar to what OOP does with class hierarchy. They are disfavored because the switch statements are scattered all over the code whereas the class construct with overrides collects all the functionality for a given class in one place. – Erik Eidt Oct 16 '15 at 23:16
  • I presume you mean "where `enum TickType`" instead of "where `enum TickPrice`". – Erik Eidt Oct 16 '15 at 23:20
  • Given what I see with the enum, I wonder why not use an array instead of a dictionary for holding "value of each last received TickType"? You just need an array optional doubles, no? (Optional for when you don't have the last value.) – Erik Eidt Oct 16 '15 at 23:22
  • @ErikEidt I believe C# arrays are not generic, and are fixed sized. Dictionary allows me to use TickType as Key. E.g. When declared as `Dictionary LastReceived` and and I want to get the ask price, it is much readable : `var ceilingPrice = LastReceived[AskPrice]` – stromms Oct 18 '15 at 21:44
  • An array of optional double would allows the same, no? Just allocate to full size of the enum. I guess you'd have to cast the enum to int: `LastReceived[(int)BuyPrice].` Perhaps the cast is too much. Or maybe I misunderstand. no worries. – Erik Eidt Oct 19 '15 at 00:51
  • I would say enums aren't a code smell at all. Comparisons to enum constants are though. (e.g. `if (type == Types.FOO) {...}`) – biziclop Feb 05 '16 at 13:31
  • 2
    `enum Smells { A, Smelly, Smell, That, Smells, smelly }` That's the only time, IMO, that an enum is a code smell. – Eon Apr 05 '17 at 20:46
  • @user40980 Enums aren't type-safe https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/enumeration-types – Stephen J. Anderson Aug 07 '18 at 07:38
  • Note that Fowler says "rule of thumb", meaning an often useful rule, but not necessarily all the time. – Dave Cousineau Oct 17 '18 at 22:20
  • (The more I think of it though, the more I think that enums are *worse* than a smell. From an OOP point of view, they are pretty much 100% bad. It's pretty much *always* better to model your concept with something that can actually be extended. I guess really they violate the open part of the open-closed principle?) – Dave Cousineau Oct 17 '18 at 22:31
  • Best practices are not a one-size fits all thing anyway. Every project is different. As for enums, I find I used them the most as `[System.Flags]` form for easy maintenance of on/off combinations. But I don't know that I've ever seen large-scale software in OOP that doesn't have SOME enums. Sometimes things should _not_ be extensible. – Jesse Williams May 07 '20 at 19:40
  • Just because it's written in a book doesn't make it true. Don't trust anyone who calls himself "Uncle". – gnasher729 Jul 24 '20 at 00:30

7 Answers7

65

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.

Karl Bielefeldt
  • 146,727
  • 38
  • 279
  • 479
  • 13
    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 03: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
  • 3
    @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
  • 8
    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
  • @Jules: When using a polymorphic class, let's say for `DaysOfWeek`, that means you're going to have to implement all Monday-specific logic in the polymorphic Monday class. That's not going to be readable nor clear. It makes sense in cases where an enum has a single well-defined use, but when an enum is widely used, it makes no sense to stack its effects into separate "enum" classes. It completely destroys readability for a given "do certain things for certain days fo the week" structure, and instead gives you readability for "what does Monday do?", which is going to predominantly be inferior. – Flater Jul 17 '19 at 14:06
  • @Jules: And that's not even getting started on the DRY violations if the same logic needs to be executed for Monday, Tuesday and Friday. How would you cover such cases, if not by checking the specific values on a case by case basis? And when you check values on a case by case basis, you're bound to the "I haven't yet checked for this newly introduced value" issue you're trying to fix. – Flater Jul 17 '19 at 14:07
  • @Jules https://en.wikipedia.org/wiki/Expression_problem polymorphic types only solve one side of it – jk. Jul 17 '19 at 14:12
  • 2
    @DaveCousineau: The answer specified _"Things that are both highly stable and **representable by a simple value**"_. If you're talking about bundling multiple values for a given entry (e.g. month name, month short name, number of days, ...), then it's no longer a simple value. The answer already explicitly precludes the issue you're trying to point out. – Flater May 07 '20 at 14:43
  • 1
    @Flater lots of things are "representable by a single value". the "highly stable" part might be more important, because you better know for a fact that your type will never need extension. these days I exclusively use my own custom reference "enums", and can pack as much info into them as I need, such as short/long names, colors, numeric values, etc. normal enums are IMO an anti-OOP concept. or really it's not even just my opinion; they are literally anti-OOP by their very implementation. they automatically prevent you from creating more useful abstractions. – Dave Cousineau May 07 '20 at 18:10
  • @Jules: I'll laugh at your first comment. – gnasher729 Jul 24 '20 at 00:31
  • @DaveCousineau: The example you use is a good reason to avoid enums. But that doesn't say anything about cases where a simple value does suffice. There are some closed value lists that have no function other than their discrete value, and enums are intended for those cases. There are no absolutes. Some cases require additional complexity or abstraction, other cases don't. It's not about right or wrong, it's about appropriate or not. – Flater Jul 24 '20 at 02:15
  • 1
    @Flater I see that as the problem. as soon as you decide "this just needs a simple value with no further extension ever needed", then you've written into both your mind and the code that this will never need extension. it's anti-OOP, at least the way they're implemented in C#. it's a dead-end type. in very simple or lower-level use cases, they might be fine, but I don't see a valid place for them in a high-level design of a large system. custom OOP-enabled "enums" can do so much more and are so much more flexible. – Dave Cousineau Jul 24 '20 at 02:48
  • 2
    @DaveCousineau: The point is that enums aren't objects, nor are they intended to be. "not OOP" isn't the same as "anti-OOP". Int addition isn't OOP either but that doesn't mean that OOP codebases should avoid using int addition, right? Same applies to enums. Just because something isn't part of OOP doesn't mean that it can't/shouldn't be part of an OOP codebase. I agree that enums are a dead-end type, **by design**. And besides, they're really just syntactic sugar over what would otherwise be a magic int (or guid). – Flater Jul 24 '20 at 08:40
  • 1
    @Flater an int is a primitive. an enum is an un-extendable user-defined type. IMO an extendable user-defined type should always be preferred over an un-extendable one. I see them as left-overs from C/C++. they might be necessary/useful for Win32/pinvoke stuff and things like that, but I don't think any larger-scale modern system should be using them. there are almost always better ways to design your system. even just from a convenience level, I can plug my own enum types into things so much easier, since they have actual ID and Name properties, and I can let them implement an INamed interface – Dave Cousineau Jul 24 '20 at 09:01
  • 1
    @DaveCousineau: Every enum can be replaced by or cast to an `int` at all times. Even in memory, they are only represented by the bits of an int (same principle applies for enums with different underlying types, e.g. guid). The enum-ness of a value is pure syntactic sugar for a developer.. You keep thinking of enums as object types, but they are in fact a closed collection of primitive values. The "**closed**" in "closed collection" is key to understanding the purpose of an enum, as that "closed" modifier explicitly excludes using it for an extendable set of values. – Flater Jul 24 '20 at 09:05
  • 1
    @Flater I know all of that, and none of it sounds the least bit useful to me from a data-modeling point of view. There are better ways of doing all of that. – Dave Cousineau Jul 24 '20 at 09:22
  • 2
    @DaveCousineau: More intricate/configurable/extendable ways of doing it? Yes. Is that more intricate/configurable/extendable way more appropriate for your use case? Presumably, if your context warrants it. A _universally better_ way of doing it? **No.** Even you yourself state that enums serve a purpose in lower-level use cases. Trying to hold _all_ code to a certain standard because _some_ code warrants it is nothing short of overzealous application of ideas and principles. A Lamborghini is a better car than a Fiat 500, but that doesn't mean I should only ever drive Lamborghinis. – Flater Jul 24 '20 at 09:30
  • 1
    @Flater a struct or class with static members is pretty close to being "universally better" than an enum. enums are a code smell because they put conceptual roadblocks into your code that don't need to be there. they encourage you to stop thinking about your model and limit your options. they are generally just bad. whenever you see one used, you should think "how could this be modelled better" and there's almost guaranteed to be ways of improving it. usually there is code scattered all over your system that belongs in the enum, but normally it can't go there. – Dave Cousineau Jul 24 '20 at 17:29
30

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.

Winston Ewert
  • 24,732
  • 12
  • 72
  • 103
  • 1
    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
  • 3
    @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 03:26
  • @WinstonEwert and with the edit, it appears that they *are* different behaviors. –  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
  • @MichaelT That's where my problem lies. The API feeds me an enum, a tuple of a Ticktype and a double. How would you connect logic variation without a switch? – stromms Oct 20 '15 at 19:07
  • @stromms `Item.matchOrder()` It is then able to look at what it is and decide if it should match the buy order or sell order. IBidSize and IBidPrice both extend an abstract class (AbstractBidTicker) that implements matchOrder() with match sell price functionality. AbstractBidTicker implements AbstractTicker that has an interface ITicker that establishes matchOrder. And now your method gets an ITicker and calls matchOrder and everything else happens. –  Oct 20 '15 at 19:13
  • (if you can't tell, I'm a JEE guy). –  Oct 20 '15 at 19:14
  • @MichaelT I can't tell at all. One big difference betwen C# and JEE is that JEE enums and C# enums are way different. A C# enum doesn't have behavior. I'm not sure if it's possible in JEE but an `interface` cannot inherit an `abstract` class. Regarding your answer, I believe I still have to make a factory which will contain the `switch` that will give the appropriate classes for the proper enum value. I just can't eliminate the switch, as was said in Karl's answer since I'm making a wrapper. – stromms Oct 20 '15 at 21:16
  • @stromms I was more poking fun at the tendency for developers who work on enterprise software to create a dozen classes and interfaces before actually doing anything. An abstract class can implement an interface. The key point that Winston makes is "TickType doesn't correspond to different behaviors" - and the enum is ok. But when there is behavior associated with it, and you start finding yourself writing switch statements all over your code (rather than hidden in a factory), that's when enums start to smell. –  Oct 20 '15 at 21:20
  • 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
  • +1 for reminding us what a code smell really is. – J.G. Jul 09 '20 at 17:45
14

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.
Kasper van den Berg
  • 2,636
  • 1
  • 16
  • 32
  • 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
  • One switch, ideally in a factory, is fine. More than one is bad, and the more of them there are, the worse it is. – ssmith May 11 '21 at 17:36
  • Also efficiency. Transmitting string keys is relatively slow and makes messages variable-length, transmitting compressed strings has its own problems (often requires/adds buffering; requires a fair bit of cpu, especially on embedded devices) – TLW Nov 24 '21 at 00:21
  • Confusing answer. Enums are compiled into integers so that's what you're transferring – CervEd Dec 13 '21 at 11:59
8

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.

Mecki
  • 1,818
  • 12
  • 17
7

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.

Giorgio
  • 19,486
  • 16
  • 84
  • 135
  • 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
  • 3
    @DaveCousineau: "Adding 1 method to 10 __existing__ files or 10 methods to 1 __new__ file." It seems to me intuitively clear that it is easier to add a new file with the new code than committing ten changes to ten existing files: local changes are easier to manage. If your intuition is different, we do not need to discuss any further. – Giorgio Oct 19 '18 at 05:45
  • K so you're weighting changing a file (in this case only adding a method) that's already written as a worse option than adding a new file? Yes my intuition is different. I coiuld say that adding more files increases the conceptual size of the code base whereas adding methods to existing classes only increases the conceptual size of the classes, not the code base. But really it's "six of one, half dozen of the other". There's no real difference in complexity, just in layout. (again really the main difference is that one is anti-OOP and the other isn't.) – Dave Cousineau Oct 19 '18 at 05:55
  • 5
    OOP is a tool, not an inherent quality of righteousness in the world. So if the *only* difference is that one is anti-OOP and the other isn't, then we could indeed say that they're fully equivalent. FWIW, I would also consider adding one new class to be a smaller and safer change than altering a large number of existing ones, because the latter opens up the possibility of regressions across the entire program, whereas in the former bugs will be local to the newly added operation. – Errorsatz May 07 '20 at 20:35
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

Chris
  • 177
  • 1
  • 9
2

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.

ruffin
  • 129
  • 5
  • 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 '19 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 '19 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 '19 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](https://en.wikipedia.org/wiki/Boyce%E2%80%93Codd_normal_form). – ruffin Jul 18 '19 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 '19 at 07:03
  • Karl Bielefeld makes this point in the accepted answer - this kind of mapping is unavoidable in the anti corruption layer between the outside world of values and the inside world of polymorphism. It's the composition root of your enum-free polymorphic world. In this situation, "enums should be replaced with polymorphism" doesn't apply. Some kind of explicit mapping is necessary (unless you want to get into reflection but that is tangential to an issue with enums per se). – Ant P Jul 19 '19 at 07:38
  • @AntP Appreciate the comments, but I'm not sure what you're requesting I change in the answer. After reading what's here, I thought Los Techies (and my example) were helpful specific use cases to round the existing answers out. I know Los Techies' info helped me. If you have more input, it sounds like this might be best handled in chat. – ruffin Jul 19 '19 at 11:03
  • Just commenting on the premise of your answer. You said "I'm now having a hard time thinking of a time enums are not a code smell on some level," on the basis that use cases for enums are better replaced with polymorphism. I'm just pointing out that this premise is not always applicable - and that time you're having a hard time thinking of is in composition or anti-corruption layers where conditionally handling specific values is unavoidable. – Ant P Jul 19 '19 at 11:31
  • "_that time you're having a hard time thinking of is in composition or anti-corruption layers where conditionally handling specific values is unavoidable._" But is it? **sniffsniff** ;^D Gotcha; thanks. – ruffin Jul 19 '19 at 12:15
  • Downvotes without comments make it hard to improve the answer. If there's something you don't agree with, let me know! – ruffin Nov 23 '21 at 20:21
  • Write the control software for a cordless drill and then tell me how bad enums are and how polymorphism would make everything so much better. – whatsisname May 12 '23 at 19:26
  • @whatsisname I'd say (as someone who has [dabbled in assembler in a past life](http://rufwork.com/mactari/), eg) that that's a great point, but then I worry that cordless drills probably run Windows 11, require activation, install updates, and phone home without asking now. – ruffin May 12 '23 at 19:50