Is a switch statement appropriate here, taking an enum?
There is another way of avoiding a switch
case which is using a Map
with a EnunMap
implementation which will keep the Type
as the key and the value as the Block
implementation.
When dealing with Enum
keys a EnumMap
implementation is recommended as per the Javadoc:
A specialized Map implementation for use with enum type keys.
This representation is extremely compact and efficient.
Implementation note: All basic operations execute in constant time. They are likely (though not guaranteed) to be faster than their HashMap counterparts.
Sample code when using a EnumMap
in place of a switch
where the key is the Type
enum instance and the value is a Supplier<Block>
(a constructor reference):
//During initialization
Map<Type, Supplier<Block>> registry = new EnumMap<>(Type.class);
registry.put(Type.I, IBlock::new);
registry.put(Type.L, LBlock::new);
registry.put(Type.J, JBlock::new);
registry.put(Type.Z, ZBlock::new);
...
Then you can fetch it from the spawnBlock()
by supplying the correct Type
and you would get a new instance of the Block
implementation every time due to the Supplier.get()
call at the end:
private void spawnBlock(Type type){
currentBlock = this.registry.get(type).get();
}
Switch is better than using if-else statements. In my opinion it is much cleaner code. Just compare it with the same code using if-else:
private void spawnBlock(Type type){
if(Type.I.equals(type)) {
currentBlock = new IBlock();
} else if(Type.L.equals(type)) {
currentBlock = new LBlock();
} else if(Type.J.equals(type)) {
currentBlock = new JBlock();
} else if(Type.Z.equals(type)) {
currentBlock = new ZBlock();
} else if(Type.S.equals(type)) {
currentBlock = new SBlock();
} else if(Type.T.equals(type)) {
currentBlock = new TBlock();
} else {
currentBlock = new OBlock();
}
}
But often you have the chance to use a different approach than switch or if-else. Just take a look at the other answers or look at this. It explains how you can improve your code using enum and putting the logic directly into the enum.
I've been told to avoid switch statements when possible
That is correct, but what you are doing here might be okay.
The point is: you don't want to have such switch statements all over the place. They are bad because they couple things together. And when you repeat the "same" switch pattern over and over again, that can turn into a maintenance nightmare. Because changes to your enum type ... means that you will have to look at each switch to determine if it needs to be adapted.
So, when you are able to hide such switching from most of your code (by doing it only in a few, ideally, one place) you are okay.
Nonetheless, a better approach could be to replace that whole switch statement with this one-liner:
currentBlock = type.createBlock();
In other words: why not put the knowledge directly into that enum class itself? Of course, that has other implications, such as a potential violation of the single responsibility principle.
But it feels quite natural that the enum that denotes the different types of blocks also provides a means to create such blocks (given the fact that the type is the only information that determines what kind of Block subclass you need).
And note: you don't necessarily move the switch into the enum. You can fully avoid switching here:
enum Type {
I(IBlock::new), L(LBlock::new), ...;
private Supplier<? extends Block> supplier;
private Type(Supplier<? extends Block> supplier) {
this.supplier = supplier;
}
public Block getBlock() {
return supplier.get();
}
( I didn't run the above through a compiler, so beware of typos )