Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

How to refactor this method if interface cannot be changed?

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;
    }
like image 417
mailzyok Avatar asked Dec 04 '25 06:12

mailzyok


2 Answers

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!

like image 173
fge Avatar answered Dec 05 '25 22:12

fge


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

like image 28
Carl Manaster Avatar answered Dec 05 '25 20:12

Carl Manaster



Donate For Us

If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!