Software Engineering Asked by stromms on December 21, 2021
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;
}
}
}
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:
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.
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.
Answered by Mecki on December 21, 2021
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 switch
es 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
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; } }
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.
Answered by ruffin on December 21, 2021
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:
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.
Answered by Giorgio on December 21, 2021
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 aunion
-like object which contains anenum
to indicate the type. The client receives theDataContract
and creates an object of a class in a hierarchy using theenum
value toswitch
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.
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:
Answered by Kasper van den Berg on December 21, 2021
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.
Answered by Karl Bielefeldt on December 21, 2021
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
Answered by Chris on December 21, 2021
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.
Answered by Winston Ewert on December 21, 2021
Get help from others!
Recent Answers
Recent Questions
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP