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

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

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

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 С++ следующее:

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

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

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

UPDATE:

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

Ответ 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, когда не найдено ни одного типа.

Ответ 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

Ответ 3

Другим возможным подходом было бы сделать process() (или, возможно, назвать его "doSubclassProcess()", если это прояснит ситуацию) abstract (в 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;
   }
}

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

Ответ 4

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

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

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??"

Положите иначе, предположительно недостижимые коды кода должны генерировать исключение, а не приводить к поведению undefined.

Ответ 5

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

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

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