How to refactor this method if interface cannot be changed?

Question

How to refactor the following method if Event interface cannot be modified? PMD report too complex, and findbugs report ITC_INHERITANCE_TYPE_CHECKING. There are also magic number such as 3, 4, 5 and so on.

 public int getEventCode(Event event) {
        if (event instanceof OneEvent) {
            return 1;
    }
        if (event instanceof TwoEvent) {
            return 2;
        }
        if (event instanceof ThreeEvent) {
            return 3;
        }
        if (event instanceof FourEvent) {
            return 4;
        }
        if (event instanceof FiveEvent) {
            return 5;
        }
        if (event instanceof SixEvent) {
            return 6;
        }
        if (event instanceof SevenEvent) {
            return 7;
        }
        if (event instanceof EightEvent) {
            return 8;
        }
        if (event instanceof NineEvent) {
            return 9;
        }
        if (event instanceof TenEvent) {
            return 10;
        }
        return event.getClass().hashCode() + 10;
    }

Answer 1

You can use a List<Class<?>> for instance:

private static final List<Class<? extends Event>> EVENT_CLASSES
    = Arrays.asList(OneEvent.class, ...);

Then:

public int getEventCode(final Event event)
{
    final Class<? extends Event> c = event.getClass();
    final int index = EVENT_CLASSES.indexOf(c);
    return index != -1 ? index + 1 : c.hashCode() + 10;
}

NOTE: requires that events are of the exact class, not derived (ie, OneEvent and not OneDerivedEvent). Otherwise the test is a little more complicated but still doable.

As to:

findbugs report ITC_INHERITANCE_TYPE_CHECKING

Yes, it is because of the instanceof checks.

However: there is a fundamental flaw with the code to begin with. there is no guarantee that .hashCode() returns the same value between two different JVM executions. What is more, it is allowed to return negative values. Which means it can return, for instance, -4 as a value; which means 6 will be returned for "other events" and therefore clash with SixEvent.

Consider a refactoring!

Answer 2

public int getEventCode(OneEvent event) {
    return 1;
}
public int getEventCode(TwoEvent event) {
    return 2;
}
// etc.

This isn't good - but if you can't change the Event class, this is probably the most object-oriented way to address your requirements. This is replacing conditional with polymorphism, without changing the class in question

Answer 3

Are there a few of these type blocks floating around the code base? If so Replacing Conditional With Polymorphism http://refactoring.com/catalog/replaceConditionalWithPolymorphism.html might help.

Answer 4

Well, it's not good to use instanceof, but if you can't change the Event Interface even to add a "int getType()" or similar, at least you could restructure your code:

Use "else if" structure instead of just if structure. I would recommend putting the Event Types that are deeper inherited first, and consider the more general event declarations last (if you test event instanceof Event you will always become true).

This way at least you reduce complexity.

Assuming all your events directly implement Event Interface and there aren't other inheritance issues to check:

public int getEventCode(Event event) {
    int result = event.getClass().hashCode() + 10;
    if (event instanceof OneEvent) {
        result = 1;
    }
    else if (event instanceof TwoEvent) {
        result = 2;
    }
    else if (event instanceof ThreeEvent) {
        result = 3;
    }
    else if (event instanceof FourEvent) {
        result = 4;
    }
    else if (event instanceof FiveEvent) {
        result = 5;
    }
    else if (event instanceof SixEvent) {
        result = 6;
    }
    else if (event instanceof SevenEvent) {
        result = 7;
    }
    else if (event instanceof EightEvent) {
        result = 8;
    }
    else if (event instanceof NineEvent) {
        result = 9;
    }
    else if (event instanceof TenEvent) {
        result = 10;
    }
    return result;
}

I also changed your method so it will only declare one return variable, this is normally considered as a better coding practice as it also helps to reduce complexity, even though it has no real technical benefit in this particular case.

But as I said, if TenEvent extends FiveEvent you can also have issues. In that case I would recommend to use the Class of the event instance you receive, something like this:

public int getEventCode(Event event) {
    int result = event.getClass().hashCode() + 10;
    String eventClass = event.getClass().getSimpleName();
    if ("OneEvent".equals(eventClass) {
        result = 1;
    }
    else if ("TwoEvent".equals(eventClass)) {
        result = 2;
    }
    return result;
}

I got lazy and am only writing the first two in the sample so you get the idea...

Or even nicer:

public int getEventCode(Event event) {
    int result = event.getClass().hashCode() + 10;
    Class eventClass = event.getClass();
    if (OneEvent.class.equals(eventClass) {
        result = 1;
    }
    else if (TwoEvent.class.equals(eventClass)) {
        result = 2;
    }
    return result;
}