Как изменить этот метод, если интерфейс не может быть изменен?

Question

Как провести рефакторинг следующего метода, если интерфейс событий нельзя изменить? Отчет PMD слишком сложный, а отчет об ошибках поиска ITC_INHERITANCE_TYPE_CHECKING. Есть также магические числа, такие как 3, 4, 5 и так далее.

 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

Вы можете использовать List<Class<?>> например:

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

Затем:

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;
}

ПРИМЕЧАНИЕ: требует, чтобы события были точного класса, а не производного (т. Е. OneEvent и не OneDerivedEvent). В противном случае тест немного сложнее, но все же выполнимо.

Относительно:

отчет о найденных ошибках ITC_INHERITANCE_TYPE_CHECKING

Да, это из-за instanceof чеки.

Однако, с самого начала существует фундаментальный недостаток кода. нет никаких гарантий, что .hashCode() возвращает одно и то же значение между двумя различными исполнениями JVM. Более того, разрешено возвращать отрицательные значения. Это означает, что он может вернуть, например, -4 в качестве значения; что означает 6 будет возвращено для "других событий" и, следовательно, столкновение с SixEvent,

Рассмотрим рефакторинг!

Answer 2

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

Это нехорошо - но если вы не можете изменить класс Event, это, вероятно, самый объектно-ориентированный способ удовлетворения ваших требований. Это замена условного с полиморфизмом, без изменения рассматриваемого класса

Answer 3

Есть ли несколько таких блоков типа, плавающих вокруг базы кода? Если это так, то может помочь замена условного на полиморфизм http://refactoring.com/catalog/replaceConditionalWithPolymorphism.html.

Answer 4

Что ж, не рекомендуется использовать instanceof, но если вы не можете изменить интерфейс событий, даже добавив int getType() или подобное, по крайней мере, вы можете реструктурировать свой код:

Используйте структуру "else if" вместо структуры if. Я бы порекомендовал сначала поместить типы событий, которые более глубоко унаследованы, и рассмотреть более общие объявления событий последними (если вы протестируете экземпляр события Event, вы всегда станете истинным).

Таким образом, по крайней мере, вы уменьшаете сложность.

Предполагая, что все ваши события непосредственно реализуют интерфейс событий, и нет других проблем наследования, которые нужно проверить:

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;
}

Я также изменил ваш метод, чтобы он объявлял только одну возвращаемую переменную, что обычно считается лучшей практикой кодирования, поскольку также помогает снизить сложность, даже если в данном конкретном случае у него нет реальных технических преимуществ.

Но, как я уже сказал, если TenEvent расширяет FiveEvent, у вас также могут возникнуть проблемы. В этом случае я бы рекомендовал использовать класс экземпляра события, который вы получаете, что-то вроде этого:

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;
}

Я поленился и пишу только первые два в примере, чтобы вы поняли...

Или даже лучше

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;
}