6

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.

  • 5
    Switching on type isn't inherently bad. There's a trade-off there - it's easier to add new functions that operate on that set of types (just write a new function) but harder to add new types (every existing function needs to be fixed). With objects you have the converse - it's easy to add new types (just implement every method) but harder to add new methods (must fix every existing class.) – Doval Jun 3 '14 at 16:31
  • Looking at the sample, couldn't you just use overloading, passing an actual type instead of a string? Your design is kind of setting off some alarms; there is code up above deciding what type string to pass in, but why defer that decision? Instead of choosing a type string up above, why not just invoke the appropriate method? In a language with function pointers, you could select a function instead of a string. But I'm wondering why you need to defer the actual execution instead of just calling it up front & keeping your call stack small. – Rob Jun 3 '14 at 17:35
  • @Doval: Very good observations. I would add that using a switch statement is not object-oriented. The object-oriented solution is to use dynamic dispatch. – Giorgio Jun 3 '14 at 17:44
  • Thanks - @Doval, one of the other downsides to switching on types is that I might forget to account for one of my types, but if I enforce it by making it a part of an explicit interface, there are ways I can ensure I won't forget it. – Taytay Jun 5 '14 at 12:27
  • @RobY, the code that calls this is not aware at compile time what type it is being called on. In this case the string is coming from the incoming serverObject, and those are untyped plain old javascript objects. – Taytay Jun 5 '14 at 12:29
2

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.

  • 1
    I was going to delete my answer, because I don't think it's quite responsive, but I'll hang onto it for the discussion. Type checking weakens OO, because the compiler and runtime engine is supposed to handle that for you. You end up doing dynamic dispatch manually. So I have to disagree -- in strict OO, it is a bad thing. In practical OO, though, sometimes it's better than the alternative. – Rob Jun 3 '14 at 17:27
  • 1
    True, doing instanceof weakens type checking, but again, this is simply an unfortunate side effect of mainstream languages not providing a real mechanism for classifying values that doesn't involve new types. But there's type-safe ways to encode this kind of classification: see this Stack Overflow question. In addition to that, the Visitor Pattern is also a poor attempt at pulling this kind of classification and switching off. – Doval Jun 3 '14 at 17:32
  • 1
    As a real, non-academic example of where this sort of thing would be necessary, suppose your application serializes/deserializes some JSON and there's only 3 valid configurations of fields. If you refuse to classify the values into the 3 kinds, you'll end up creating one monolithic class that contains the union of all fields of all 3 configurations, and adding an enum for figuring out which of the 3 configurations this value has. Then the burden of reading the right fields falls squarely on the programmer. – Doval Jun 3 '14 at 17:37
  • 2
    No one said OO was good. :) – Rob Jun 3 '14 at 17:38
  • 1
    @Doval: As you pointed out, functional languages use pattern matching. Indeed, pattern-matching is the functional approach, dynamic dispatching the object-oriented approach. – Giorgio Jun 3 '14 at 17:46
1

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.

  • By the way, is this ActionScript? I wasn't sure what language I was working in so the syntax may not be 100%. – PhilDin Jun 3 '14 at 17:04
  • Thanks @PhilDin - it's actually TypeScript. (Similar to ActionScript). The advantage of your approach is that you limit the switch statement to one place, but I still have the possibility of forgetting to create a finder for a type. Not a massive deal, but would prefer that not to be an issue. I also have to create additional classes for each of my types, so I feel like this might be a lot more code/ceremony than I wanted. – Taytay Jun 5 '14 at 12:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.