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

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

28 голосов
/ 13 января 2012

В одном из моих проектов у меня есть два "объекта передачи данных" 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 C ++ следующее:

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

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

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

ОБНОВЛЕНИЕ:

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

Ответы [ 5 ]

26 голосов
/ 13 января 2012

В таких случаях обычно используется шаблон Посетитель . Хотя код немного сложнее, но после добавления нового подкласса 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, когда ни один из типов не найден.

3 голосов
/ 13 января 2012

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

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

0 голосов
/ 13 января 2012

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

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

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

0 голосов
/ 13 января 2012

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

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

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

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

0 голосов
/ 13 января 2012

Другим возможным подходом было бы сделать 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;
   }
}

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

...