I’m seeing some code like this in our code base, and want to refactor it:
(Typescript psuedocode follows):
class EntityManager{
private findEntityForServerObject(entityType:string, serverObject:any):IEntity {
var existingEntity:IEntity = null;
switch(entityType) {
case Types.UserSetting:
existingEntity = this.getUserSettingByUserIdAndSettingName(serverObject.user_id, serverObject.setting_name);
break;
case Types.Bar:
existingEntity = this.getBarByUserIdAndId(serverObject.user_id, serverObject.id);
break;
//Lots more case statements here...
}
return existingEntity;
}
}
The downsides of switching on type are self-explanatory. Normally, when switching behavior based on type, I try to push the behavior into subclasses so that I can reduce this to a single method call, and let polymorphism take care of the rest.
However, the following two things are giving me pause:
1) I don’t want to couple the serverObject with the class that is storing all of these objects. It doesn’t know where to look for entities of a certain type. And unfortunately, the identity of a type of ServerObject varies with the type of ServerObject. (So sometimes it’s just an ID, other times it’s a combination of an id and a uniquely identifying string, etc). And this behavior doesn’t belong down there on those subclasses. It is the responsibility of the EntityManager and its delegates.
2) In this case, I can’t modify the ServerObject classes since they’re plain old data objects. It should be mentioned that I’ve got other instances of the above method that take a parameter like “IEntity” and proceed to do almost the same thing (but slightly modify the name of the methods they’re calling to get the identity of the entity). So, we might have:
case Types.Bar:
existingEntity = this.getBarByUserIdAndId(entity.getUserId(), entity.getId());
break;
So in that case, I can change the entity interface and subclasses, but this isn’t behavior that belongs in that class.
So, I think that points me to some sort of map. So eventually I will call:
private findEntityForServerObject(entityType:string, serverObject:any):IEntity {
return aMapOfSomeSort[entityType].findByServerObject(serverObject);
}
private findEntityForEntity(someEntity:IEntity):IEntity {
return aMapOfSomeSort[someEntity.entityType].findByEntity(someEntity);
}
Which means I need to register some sort of strategy classes/functions at runtime with this map. And again, I darn well better remember to register one for each my my types, or I’ll get a runtime exception.
Is there a better way to refactor this? I feel like I’m missing something really obvious here.
7
Whenever you start switching by type (or providing if’s based on type), then you’re going against the OO model. In a good OO design, the implementation class should be hidden from the calling code.
(That said, there are times when it’s easier to do a quick instanceof, but I try to make it rare).
Polymorphism is the answer here.
Really, what you’re doing is calling some common action on some classes. So spin off an interface that defines that common call, and then reference the objects through that interface.
In other words, whenever I see myself writing “if xxx.class” or “xxx instanceof” or “switch (xxx.class)”, I ask myself is there’s a common interface I can pull out.
Now, it can be tricky. If the different classes require different parameters, you might need to use some sort of Builder pattern. It can get complicated.
But the short answer, anyway, is: if you’re switching or iffing based on class type, you should really be using polymorphism instead.
13
I would map between entityTypes and an object that knows what to do with that entity type. So you might have something like
private findEntityForServerObject(entityType:string, serverObject:any):IEntity {
EntityFinder entityFinder = EntityFinder.getForEntity(entityType);
return entityFinder.find(serverObject);
}
You will then be pushing the switch into EntityFinder which will have a static method along the following lines
class EntityFinder {
public static EntityFinder getForEntity(String entityType) {
switch (entityType) {
case Types.UserSetting:
return new UserSettingsFinder();
case Types.Bar:
return new BarSettingsFinder();
default:
// Possibly better than a null pointer
return new UnknownSettingsFinder();
}
}
}
If you introduce a new type, you will have to remember to include it in the switch but hopefully, you will only have one place to do this.
2