'Multiple If-else or enum - which one is preferable and why? [closed]
Here is the original code:
public class FruitGrower {
public void growAFruit(String type) {
if ("wtrmln".equals(type)) {
//do watermelon growing stuff
} else if ("ppl".equals(type)) {
//do apple growing stuff
} else if ("pnppl".equals(type)) {
//do pineapple growing stuff
} else if ("rng".equals(type)) {
//do orange growing stuff
} else {
// do other fruit growing stuff
}
}
}
This is how I changed it:
public class FruitGrower {
enum Fruits {
WATERMELON {
@Override
void growAFruit() {
//do watermelon growing stuff
}
},
APPLE {
@Override
void growAFruit() {
//do apple growing stuff
}
},
PINEAPPLE {
@Override
void growAFruit() {
//do pineapple growing stuff
}
},
ORANGE {
@Override
void growAFruit() {
//do orange growing stuff
}
},
OTHER {
@Override
void growAFruit() {
// do other fruit growing stuff
}
};
static void grow(String type) {
if ("wtrmln".equals(type)) {
WATERMELON.growAFruit();
} else if ("ppl".equals(type)) {
APPLE.growAFruit();
} else if ("pnppl".equals(type)) {
PINEAPPLE.growAFruit();
} else if ("rng".equals(type)) {
ORANGE.growAFruit();
} else {
OTHER.growAFruit();
}
};
abstract void growAFruit();
}
public void growAFruit(String type) {
Fruits.grow(type);
}
}
I see that enums code is longer and may be not as clear as if-else code, but I believe it's better, could someone tell me, why I'm wrong (or maybe I'm not)?
Are there any concerns about using enum instead of if-else?
Solution 1:[1]
You've already got good answers about improving your use of Enums. As for why they're better than string constants:
I think the biggest benefit is compile-time error checking. If I were to call growAFruit("watermelon"), the compiler would have no idea anything was wrong. And since I've spelled it correctly, it's not going to stand out as a mistake when you're viewing the code. But if I were to WATERMELEN.growAFruit(), the compiler can tell you immediately that I've misspelled it.
You also get to define growAFruit as a bunch of simple, easy-to-read methods, instead of a big block of if-then-elses. This becomes even more apparent when you have a few dozen fruits, or when you start adding harvestAFruit(), packageAFruit(), sellAFruit(), etc. Without enums you'd be copying your big if-else block all over, and if you forgot to add a case it would fall through to the default case or do nothing, while with enums the compiler can tell you that the method hasn't been implemented.
Even more compiler-checking goodness: If you also have a growVegetable method and the related string constants, there's nothing stopping you from calling growVegetable("pineapple") or growFruit("peas"). If you've got a "tomato" constant, the only way to know if you consider it a fruit or a vegetable is to read the source code of the relevant methods. Once again, with enums the compiler can tell you right away if you've done something wrong.
Another benefit is that it groups related constants together and gives them a proper home. The alternative being a bunch of public static final fields thrown in some class that happens to use them, or stuck a interface of constants. The interface full of constants doesn't even make sense because if that's all you need defining the enum is much easier than writing out the interface. Also, in classes or interfaces there is the possibility to accidentally use the same value for more than one constant.
They're also iterable. To get all the values of an enum you can just call Fruit.values(), whereas with constants you'd have to create and populate your own array. Or if just using literals as in your example, there is no authoritive list of valid values.
Bonus Round: IDE Support
- With an enum, you can use your IDE's auto-completion feature and automated refactorings
- You can use things like "Find References" in Eclipse with enum values, while you'd have to do a plain text search to find string literals, which will usually also return a lot of false-positives (event if you use static final constants, someone could have used the string literal somewhere)
The main reason not to use an enum would be if you don't know all the possible values at compile time (i.e. you need to add more values while the program is running). In that case you might want to define them as a class heirarchy. Also, don't throw a bunch of unrelated constants into an enum and call it a day. There should be some sort of common thread connecting the values. You can always make multiple enums if that's more appropriate.
Solution 2:[2]
Enums are the way to go, but you can dramatically improve your code like this:
public static String grow(String type) {
return Fruits.valueOf(type.toUpperCase()).gimmeFruit();
};
Oh, you need a default case, that makes it a bit tougher. Of course you can do this:
public static String grow(String type) {
try{
return Fruits.valueOf(type.toUpperCase()).gimmeFruit();
}catch(IllegalArgumentException e){
return Fruits.OTHER.gimmeFruit();
}
};
But that's pretty ugly. I guess I'd to something like this:
public static String grow(String type) {
Fruits /*btw enums should be singular */ fruit = Fruits.OTHER;
for(Fruits candidate : Fruits.values()){
if(candidate.name().equalsIgnoreCase(type)){
fruit = candidate;
break;
}
}
return fruit.gimmeFruit();
};
Also, if all your enum methods do is return a value, you should refactor your design so that you initialize the values in a constructor and return them in a method defined in the Enum class, not the individual items:
public enum Fruit{
WATERMELON("watermelon fruit"),
APPLE("apple fruit")
// etc.
;
private final String innerName;
private Fruit(String innerName){ this.innerName = innerName; }
public String getInnerName(){ return this.innerName; }
}
Solution 3:[3]
You have only made half of the changes to be cleaner. The grow method should be changed like this:
static String grow(Fruits type) {
return type.gimmeFruit();
}
And Fruits should be renamed to Fruit: an apple is a fruit, not a fruits.
If you really need to keep your string types, then define a method (in the enum class itself, for example) returning the Fruit associated to each type. But most of the code should use Fruit instead of String.
Solution 4:[4]
I think you want a Map<String, Fruit> (or <String, FruitGrower>).
This map could be filled automatically by the enum's constructors, or by a static initializer. (It could even map multiple names on the same enum constant, if some fruits have alias names.)
Your grow method then looks like this:
static void grow(String type) {
Fruit f = map.get(type);
if (f == null) {
OTHER.growFruit();
}
else {
f.growFruit();
}
}
Of course, do you really need the string here? Shouldn't you always use the enum object?
Solution 5:[5]
I'm not sure I'd use Enums here. I may be missing something here (?), but I think my solution would look something like this, with separate classes for each type of fruit, all based on one genera fruit-class:
// Note: Added in response to comment below
public enum FruitType {
WATERMELON,
WHATEVERYOUWANT,
....
}
public class FruitFactory {
public Fruit getFruitToGrow(FruitType type) {
Fruit fruitToGrow = null;
switch(type){
case WATERMELON:
fruitToGrow = new Watermelon();
break;
case WHATEVERYOUWANT:
...
default:
fruitToGrow = new Fruit();
}
return fruitToGrow;
}
}
public class Fruit(){
public void grow() {
// Do other fruit growing stuff
}
}
// Add a separate class for each specific fruit:
public class Watermelon extends Fruit(){
@override
public void grow(){
// Do more specific stuff...
}
}
Solution 6:[6]
I think you're on the right track. I'd go and throw in some extra bytes for a HashMap to get rid of the string switching block. This gives you both, cleaner looks, less code and most likely a little extra performance.
public enum Fruit {
APPLE("ppl") {
public void grow() {
// TODO
}
},
WATERMELON("wtrmln") {
public void grow() {
// TODO
}
},
// SNIP extra vitamins go here
OTHER(null) {
public void grow() {
// TODO
}
};
private static Map<String, Fruit> CODE_LOOKUP;
static {
// populate code lookup map with all fruits but other
Map<String, Fruit> map = new HashMap<String, Fruit>();
for (Fruit v : values()) {
if (v != OTHER) {
map.put(v.getCode(), v);
}
}
CODE_LOOKUP = Collections.unmodifiableMap(map);
}
public static Fruit getByCode(String code) {
Fruit f = CODE_LOOKUP.get(code);
return f == null ? OTHER : f;
}
private final String _code;
private Fruit(String code) {
_code = code;
}
public String getCode() {
return _code;
}
public abstract void grow();
}
And that's how you use it:
Fruit.getByCode("wtrmln").grow();
Simple, no need for a FruitGrower, but go for it if you think it's necessary.
Solution 7:[7]
I second Sean Patrick Floyd on that enums are the way to go, but would like to add that you can shorten your code event more dramatically by using a scheme like this:
enum Fruits {
WATERMELON("watermelon fruit"),
APPLE("apple fruit"); //...
private final String gimme;
private Fruits(String gimme) {
this.gimme = gimme;
}
String gimmeFruit() { return this.gimme; }
}
Also, the "grow" method is suspicious. Shouldn't it be something like
public static String grow(Fruits f) {
return f.gimmeFruit();
}
Solution 8:[8]
You can also improve it by using a variable to store the gimmeFruit value and inititialize with the constructor.
(I haven't actually compiled this so there may be some syntax errors)
public class FruitGrower {
enum Fruits {
WATERMELON("watermelon fruit"),
APPLE("apple fruit"),
PINEAPPLE("pineapple fruit"),
ORANGE("orange fruit"),
OTHER("other fruit")
private String gimmeStr;
private Fruits(String gimmeText) {
gimmeStr = gimmeText;
}
public static String grow(String type) {
return Fruits.valueOf(type.toUpperCase()).gimmeFruit();
}
public String gimmeFruit(String type) {
return gimmeStr;
}
}
}
EDIT: If the type for the grow method is not the same string, then use a Map to define the matches of type to Enum and return the lookup from the map.
Solution 9:[9]
I often implement a method in enums parsing a given String and gives back the corresponding enum constant. I always name this method parse(String).
Sometimes I overload this method in order to parse enum constant by another given input type, too.
It's implementation is always the same:
Iterate over all enum values() and return when you hit one. Finally do a return as fallthrough - often a specific enum constant or null. In most cases I prefer null.
public class FruitGrower {
enum Fruit {
WATERMELON("wtrmln") {
@Override
void grow() {
//do watermelon growing stuff
}
},
APPLE("ppl") {
@Override
void grow() {
//do apple growing stuff
}
},
PINEAPPLE("pnppl") {
@Override
void grow() {
//do pineapple growing stuff
}
},
ORANGE("rng") {
@Override
void grow() {
//do orange growing stuff
}
},
OTHER("") {
@Override
void grow() {
// do other fruit growing stuff
}
};
private String name;
private Fruit(String name) {
this.name = name;
}
abstract void grow();
public static Fruit parse(String name) {
for(Fruit f : values()) {
if(f.name.equals(name)){
return f;
}
}
return OTHER; //fallthrough value (null or specific enum constant like here.)
}
}
public void growAFruit(String name) {
Fruit.parse(name).grow();
}
}
If you do not really need this Fruit.OTHER then delete it. Or how a "Other-fruit" grows? oO
Return null then in parse(String) method as fallthrough value and do null-check before calling grow() in growAFruit(String).
It is a good idea to add @CheckForNull annotation to the parse(String) method then.
Solution 10:[10]
The public API is exactly the same. You still have the same if-else block, it's just in the enum now. So I think it's no better. If anything it's worse, due to added complexity.
What does it mean to 'do fruit growing stuff'? Are you talking about stuff the fruit grower does (till the soil, plant seeds, etc), or stuff the fruit itself does (germinate, sprout, blossom, etc)? In the original example, the actions are defined by the FruitGrower, but in your modifications they are defined by the Fruit. This makes a big difference when you consider subclassing. For example I might want to define a MasterFruitGrower who uses different processes to grow fruit better. Having the grow() operation defined in Fruit makes this harder to reason about.
How complex are the fruit growing operations? If you're concerned about the line length of the if-else block, I think a better idea is to define separate fruit growing methods (growWatermelon(), growOrange(), ...) or define a FruitGrowingProcedure interface, implementing subclasses for each fruit type, and store them in a map or set under FruitGrower.
Solution 11:[11]
The answer to your question is using enums, or better yet, factories and polymorphism, as mentioned above. However, if you want to get rid of switches (which is what your if-else statement is really doing) completely , a good way, if not the best, to do it is using inversion of control. Thus, I suggest using spring, as follows:
public interface Fruit {
public void grow();
}
public class Watermelon implements Fruit {
@Override
public void grow()
{
//Add Business Logic Here
}
}
Now, create the fruit locator interface, as follows:
public interface FruitLocator {
Fruit getFruit(String type);
}
Let main class have a reference to a FruitLocator object, a setter for it, and just call the getFruit command:
private FruitLocator fruitLocator;
public void setFruitLocator (FruitLocator fruitLocator)
{
this.fruitLocator = fruitLocator;
}
public void growAFruit(String type) {
fruitLocator.getFruit(type).grow();
}
Now comes the tricky part. Define your FruitGrower class as a spring bean, as well as your FruitLocator and Fruits:
<bean id="fruitGrower" class="yourpackage.FruitGrower">
<property name="fruitLocator" ref="fruitLocator" />
</bean>
<bean id="fruitLocator"
class="org.springframework.beans.factory.config.ServiceLocatorFactoryBean">
<property name="serviceLocatorInterface"
value="yourpackage.FruitLocator" />
<property name="serviceMappings" ref="locatorProperties" />
</bean>
<bean id="locatorProperties"
class="org.springframework.beans.factory.config.PropertiesFactoryBean">
<property name="location" value="classpath:fruits.properties" />
</bean>
<bean id="waterMelon" class="yourpackage.WaterMelon">
</bean>
Only thing left to do is create a fruits.properties file in your classpath and add the type-bean mapping, as follows:
wtrmln=waterMelon
Now, you can add as many fruits as you wish, you only need to create a new fruit class, define it as a bean and add a mapping to your properties file. Much much more scalable than hunting for if-else logic in the code.
I know this seems complex at first, but I think the topic would be incomplete without mentioning Inversion of Control.
Solution 12:[12]
To answer your question, I would say neither if-else or enums are preferable to your specific problem. A good way to approach this problem would be to use encapsulation and abstraction and let inheritance handle the "type" for you.
The task of growing a fruit varies greatly between each fruit. It would be wise to make each fruit its own class allowing greater flexibility later on when more functionality may be needed.
Heres a good example:
// Abstract Base Class (Everything common across all fruits)
public abstract class Fruit {
public abstract void grow();
}
// Concrete class, for each "type" of fruit
public class Apple extends Fruit {
@override
public void grow() {
// Grow an apple
}
}
public class Orange extends Fruit {
@override
public void grow() {
// Grow an orange
}
}
...
Once the product classes have been defined we can create a class that grows fruit without any "ify" type checking.
public class FruitGrower {
public void growAFruit(Fruit fruit) {
fruit.grow();
}
}
Solution 13:[13]
Well, the enums code is surely longer. What I suggest:
- Use Strings when the logic is simple, small, and will be used 1 time.
- Use Enums when you have to use the constants many times, addition or modifications are possible, or when they are mixed in a complex code so it is better to clarify them with the Enum.
Solution 14:[14]
If different Fruits have different grow behaviors, I would not use an if-else or an enum, and instead use an object-oriented design. The problem with the if-else/enum style (I consider them equivalent in your example) is that they collect behaviors of different object types into one location. If you add a new fruit, you have to edit your if-else/enum each time. This violates the open-closed principle.
Consider if your 4 fruits had 3 behaviors (e.g. grow, ripen, rot). You would have an if-else/enum for each behavior, each containing 4 fruit references. Now consider adding a 5th fruit, you must edit 3 different if-else/enum blocks. The behavior of a watermelon has nothing to do with an apple, but they are in the same piece of code. Now consider adding a new fruit behavior (e.g. aroma) - you must create a new if-else/enum, which has the same issues.
I believe the right solution in most cases is to use classes (e.g. a Fruit interface/base class with one implementation class per fruit) and put the behavior in each class instead of collecting it all in one place. So when you add a new fruit, the only code being changed is that a new fruit class is written.
There is a well-known refactoring you can use to take your existing code and migrate it to what I am talking about. It's called Replace Conditional with Polymorphism.
Sources
This article follows the attribution requirements of Stack Overflow and is licensed under CC BY-SA 3.0.
Source: Stack Overflow