Even though the last answer was a year ago, I would like to make some reviews/comments on this topic.
Answers Review
I agree with @CarlManaster about coding the switch
statement once to avoid all well known problems of dealing with duplicated code, in this case involving conditionals (some of them mentioned by @thkala).
I don't believe the approach proposed by @KonradSzałwiński or @AlexanderKogtenkov fits this scenario for two reasons:
First, from the problem you've described, you don't need to dynamically change the mapping between the name of an action and the instance of an action that handles it.
Notice these solutions allows doing that (by simply assigning an action name to a new action instance), while the static switch-based solution doesn't (the mappings are hardcoded).
Also, you'll still need a conditional to check if a given key is defined in the mapping table, if not an action should be taken (the default
part of a switch statement).
Second, in this particular example, dictionaries
are really hidden implementations of switch statement. Even more, it might be easier to read/understand the switch statement with the default clause than having to mentally execute the code that returns the handling object from the mapping table, including the handling of a not defined key.
There is a way you can get rid of all conditionals, including the switch statement:
Removing the switch statement (use no conditionals at all)
How to create the right action object from the action name?
I'll be language-agnostic so this answer doesn't get that long, but the trick is to realize classes are objects too.
If you've already defined a polimorphic hierarchy, it makes no sense to make reference to a concrete subclass of BaseAction
: why not ask it to return the right instance handling an action by its name?
That is usually implemented by the same switch statement you had written (say, a factory method)... but what about this:
public class BaseAction {
//I'm using this notation to write a class method
public static handlingByName(anActionName) {
subclasses = this.concreteSubclasses()
handlingClass = subclasses.detect(x => x.handlesByName(anActionName));
return new handlingClass();
}
}
So, what is that method doing?
First, retrieves all concrete subclasses of this (which points to BaseAction
). In your example you would get back a collection with ViewAction
, EditAction
and SortAction
.
Notice that I said concrete subclasses, not all subclasses. If the hierarchy is deeper, concrete subclasses will always be the ones in the bottom of the hierarchy (leaf). That's because they are the only ones supposed not to be abstract and provide real implementation.
Second, get the first subclass that answer whether or not it can handle an action by its name (I'm using a lambda/closure flavored notation). A sample implementation of the handlesByName
class method for ViewAction
would look like:
public static class ViewAction {
public static bool handlesByName(anActionName) {
return anActionName == 'view'
}
}
Third, we send the message new to the class that handles the action, effectively creating an instance of it.
Of course, you have to deal with the case when none of the subclass handles the action by it's name. Many programming languages, including Smalltalk and Ruby, allows passing the detect method a second lambda/closure that will only get evaluated if none of the subclasses matches the criteria.
Also, you will have to deal with the case more than one subclass handles the action by its name (probably, one of these methods was coded in the wrong way).
Conclusion
One advantage of this approach is that new actions can be supported by writing (and not modifying) existing code: just create a new subclass of BaseAction
and implementing the handlesByName
class method correctly. It effectively supports adding a new feature by adding a new concept, without modifying the existing impementation. It is clear that, if the new feature requires a new polimorphic method to be added to the hierarchy, changes will be needed.
Also, you can provide the developers using your system feedback: "The action provided is not handled by any subclass of BaseAction, please create a new subclass and implement the abstract methods". For me, the fact that the model itself tells you what's wrong (instead of trying to execute mentally a look up table) adds value and clear directions about what has to be done.
Yes, this might sound over-design. Please keep an open mind and realize that whether a solution is over-designed or not has to do, among other things, with the development culture of the particular programming language you're using. For example, .NET guys probably won't be using it because the .NET doesn't allow you to treat classes as real objects, while in the other hand, that solution is used in Smalltalk/Ruby cultures.
Finally, use common sense and taste to determine beforehand if a particular technique really solves your problem before using it. It is tempting yes, but all trade-offs (culture, seniority of the developers, resistance to change, open mindness, etc) should be evaluated.