>Refactoring: Leveraging Polymorphism

>While working on some enhancements to bring the PsychoPath Processor’s implementation of the ElementTest and AttributeTest methods into compliance with the spec, I ran across the following bit of code:

if (ktest instanceof DocumentTest)
_type_class = DocType.class;
else if (ktest instanceof ElementTest)
_type_class = ElementType.class;
else if (ktest instanceof TextTest)
_type_class = TextType.class;
else if (ktest instanceof AttributeTest)
_type_class = AttrType.class;
else if (ktest instanceof CommentTest)
_type_class = CommentType.class;
else if (ktest instanceof PITest)
_type_class = PIType.class;
else if (ktest instanceof AnyKindTest)
_type_class = NodeType.class;
else if (ktest instanceof CommentTest)
_type_class = CommentType.class;
assert false;

This piece of code screams to be refactored. It works, but it is a pain to read and if another Kind is added another conditional check needs to be added. I’m a big fan of Martin Fowler’s Refactoring book, and believe it is one reference book that everybody should have. The conditional is crying out to use the Replace Conditional with Polymorphism. Most times when instanceof is used, it usually is a sign where Polymorphism might work better.

To help enhance the implementation of ElementTest and AttributeTest I needed to do some additional processing steps in both ElementTest and AttributeTest. Both ElementTest and AttributeTest needed to have their own implementation of the methods, and all classes still needed to return the XDM data model type that it used. So with a little help from Extract Method, Move Method, and Replace Conditional with Polymorphism. We ended up with:

typeClass = ktest.getXDMClassType();
anytype = ktest.createTestType(rs);
nodeName = ktest.name();
wild = ktest.isWild();

This greatly simplified the code, and provided the needed functionality. However, what it also now has revealed an odor that was previously hidden, the Feature Envy Smell. In the SeqType class, the wild, typeclass, and anytype variables are used later in the match() method. In particular the following bit of code.

for (Iterator i = args.iterator(); i.hasNext();) {
AnyType arg = (AnyType) i.next();

// make sure all args are the same type as expected type
if (!(typeClass.isInstance(arg))) {
throw new DynamicError(TypeError.invalid_type(null));

if (anytype != null && occurrence != OCC_STAR) {
if ((nodeName != null || wild) && arg instanceof NodeType) {
NodeType nodeType = (NodeType) arg;
Node node = nodeType.node_value();
Node lnode = ((NodeType) anytype).node_value();
if (lnode == null) {
throw new DynamicError(TypeError.invalid_type(null));
if (!lnode.isEqualNode(node)) {
throw new DynamicError(TypeError.invalid_type(null));

All three values that are now stored as part of the appropriate KindTest subclasses. So another level of refactoring using Extract Method, and Move Method could probably further clean up the code.

In order to make any of these refactorings possible. It is necessary to have tests to go with this. The test suite helps verify as you are making the refactorings that you aren’t breaking the existing functionality. With out the tests to verify that everything is still working, you are rolling some dice every time you touch your code. The tests give you the saftey net to make these types of refactorings.

This entry was posted in eclipse, refactoring, testing, xml, xpath. Bookmark the permalink.

One Response to >Refactoring: Leveraging Polymorphism

  1. André says:

    >great! I frequently use this approach and wondered if I was the only one that liked it :-). The last occasion I came across this was in ECF Rest https://bugs.eclipse.org/bugs/show_bug.cgi?id=297239#c3. To be frank applying this pattern leads to better method cohesion and lower coupling (you separate cases into different classes) but it creates additional artifacts (the classes). The instanceof approach might be simpler and more effective where you have to cover a few cases only.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )


Connecting to %s