Это использование оператора instanceof считается плохим дизайном?

Question

В одном из моих проектов у меня есть два "объекта передачи данных" RecordType1 и RecordType2, которые наследуются от абстрактного класса RecordType.

Я хочу, чтобы оба объекта RecordType обрабатывались одним и тем же классом RecordProcessor в методе "process". Моей первой мыслью было создание общего метода процесса, который делегировал бы два конкретных метода процесса следующим образом:

public RecordType process(RecordType record){

    if (record instanceof RecordType1)
        return process((RecordType1) record);
    else if (record instanceof RecordType2)
        return process((RecordType2) record);

    throw new IllegalArgumentException(record);
}

public RecordType1 process(RecordType1 record){
    // Specific processing for Record Type 1
}

public RecordType2 process(RecordType2 record){
    // Specific processing for Record Type 2
}

Я читал, что Скотт Мейерс пишет в Effective C++ следующее:

"Каждый раз, когда вы обнаруживаете, что пишете код формы", если объект имеет тип T1, тогда делайте что-то, но если он имеет тип T2, тогда делайте что-то еще, "бейте себя".

Если он прав, ясно, что я должен ударить себя. Я действительно не понимаю, как это плохо дизайн (если, конечно, кто-то подклассы RecordType и добавляет в RecordType3, не добавляя еще одну строку в общий метод "Процесс", который обрабатывает его, таким образом создавая NPE), и альтернативы, которые я могу думать из-за того, что я взял на себя основную логику обработки внутри самих классов RecordType, что на самом деле не имеет особого смысла для меня, поскольку в теории может быть много разных типов обработки, которые я хотел бы выполнить с этими записями.

Может кто-нибудь объяснить, почему это может считаться плохим дизайном, и предоставить какую-то альтернативу, которая по-прежнему возлагает ответственность за обработку этих записей на класс "Обработка"?

ОБНОВИТЬ:

• Изменено return null в throw new IllegalArgumentException(record);
• Просто для пояснения, есть три причины, по которым простого метода RecordType.process() будет недостаточно: во-первых, обработка действительно слишком удалена от RecordType, чтобы заслужить свой собственный метод в подклассах RecordType. Кроме того, существует целый ряд различных типов обработки, которые теоретически могут выполняться разными процессорами. Наконец, RecordType разработан как простой DTO-класс с минимальными методами изменения состояния, определенными внутри.

Answer 1

Шаблон Visitor обычно используется в таких случаях. Хотя код немного сложнее, но после добавления нового RecordType Подкласс вы должны реализовать логику везде, иначе она не будет компилироваться. С instanceof повсюду очень легко пропустить одно или два места.

Пример:

public abstract class RecordType {
    public abstract <T> T accept(RecordTypeVisitor<T> visitor);
}

public interface RecordTypeVisitor<T> {
    T visitOne(RecordType1 recordType);
    T visitTwo(RecordType2 recordType);
}

public class RecordType1 extends RecordType {
    public <T> T accept(RecordTypeVisitor<T> visitor) {
        return visitor.visitOne(this);
    }
}

public class RecordType2 extends RecordType {
    public <T> T accept(RecordTypeVisitor<T> visitor) {
        return visitor.visitTwo(this);
    }
}

Использование (обратите внимание на общий тип возвращаемого значения):

String result = record.accept(new RecordTypeVisitor<String>() {

    String visitOne(RecordType1 recordType) {
        //processing of RecordType1
        return "Jeden";
    }

    String visitTwo(RecordType2 recordType) {
        //processing of RecordType2
        return "Dwa";
    }

});

Также я бы рекомендовал бросить исключение:

throw new IllegalArgumentException(record);

вместо возвращения null когда ни один тип не найден.

Answer 2

Мое предложение:

public RecordType process(RecordType record){
    return record.process();
}

public class RecordType
{
    public RecordType process()
    {
        return null;
    }
}

public class RecordType1 extends RecordType
{
    @Override
    public RecordType process()
    {
        ...
    }
}

public class RecordType2 extends RecordType
{
    @Override
    public RecordType process()
    {
        ...
    }
}

Если код, который нужно выполнить, связан с чем-то, что модель не должна знать (например, с пользовательским интерфейсом), тогда вам нужно будет использовать своего рода шаблон двойной отправки или шаблон посетителя.

http://en.wikipedia.org/wiki/Double_dispatch

Answer 3

Плохой дизайн на первый взгляд, как в вашем примере, не использование шаблона посетителя, когда это применимо.

Другой - это эффективность. instanceof является довольно медленным, по сравнению с другими методами, такими как сравнение с class объект с использованием равенства.

При использовании шаблона посетителя обычно используется эффективное и элегантное решение. Map для отображения между поддержкой class и экземпляр посетителя. большой if ... else блокировать с instanceof проверки будут очень неэффективными.

Answer 4

Другой возможный подход состоит в том, чтобы сделать process () (или, возможно, назвать его "doSubclassProcess()", если это проясняет ситуацию) абстрактным (в RecordType), и иметь фактические реализации в подклассах. например

class RecordType {
   protected abstract RecordType doSubclassProcess(RecordType rt);

   public process(RecordType rt) {
     // you can do any prelim or common processing here
     // ...

     // now do subclass specific stuff...
     return doSubclassProcess(rt);
   }
}

class RecordType1 extends RecordType {
   protected RecordType1 doSubclassProcess(RecordType RT) {
      // need a cast, but you are pretty sure it is safe here
      RecordType1 rt1 = (RecordType1) rt;
      // now do what you want to rt
      return rt1;
   }
}

Остерегайтесь нескольких опечаток - думаю, я исправил их все.

Answer 5

Дизайн - это средство для достижения цели, и, не зная вашей цели или ограничений, никто не может сказать, хорош ли ваш дизайн в данной конкретной ситуации или как его можно улучшить.

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

PS: в обзоре кода я бы пометил return nullпотому что это может распространять ошибки, а не сообщать о них. Рассматривать:

RecordType processed = process(new RecordType3());

// many hours later, in a different part of the program

processed.getX(); // "Why is this null sometimes??"

Иными словами, якобы недоступные пути кода должны вызывать исключение, а не приводить к неопределенному поведению.

Answer 6

Это противоречит принципу открытия-закрытия SOLID.