この「InstanceOf」オペレーターの使用は、悪い設計と見なされていますか？

https://stackoverflow.com/questions/8841577

27-10-2019
|

質問

私のプロジェクトの1つには、RecordTypeの抽象クラスから継承する2つの「データ転送オブジェクト」RecordType1とRecordType2があります。

両方のRecordTypeオブジェクトを、「プロセス」メソッド内の同じRecordProcessorクラスによって処理したいと考えています。私の最初の考えは、次の2つの特定のプロセス方法に委任する一般的なプロセス方法を作成することでした。

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
}

スコット・マイヤーズが以下を書いていることを読んだことがあります 効果的なC ++ :

「オブジェクトがタイプT1の場合、フォームのコードを書いているときはいつでも、何かをしますが、タイプT2の場合は、「自分を平手打ちしてください。」

彼が正しければ、明らかに私は自分自身を平手打ちするべきです。これがどのように悪いデザインであるかは本当にわかりません（もちろん、誰かがRecordTypeをサブクラス化し、それを処理する一般的な「プロセス」メソッドに別のラインを追加せずにRecordType3に追加して、NPEを作成しない限り）、そして私が考えることができる代替案は理論的には、これらのレコードで実行したい多くの異なるタイプの処理がある可能性があるため、特定の処理ロジックの矢面を記録クラス自体に置くことはあまり意味がありません。

これが悪いデザインと見なされる理由を誰かが説明し、これらのレコードを「処理」クラスに処理する責任をまだ与えているある種の選択肢を提供することができますか？

アップデート：

かわった return null に throw new IllegalArgumentException(record);
明確にするために、単純なRecordType.Process（）メソッドが十分ではない3つの理由があります。まず、記録型サブクラスに独自の方法に値するには、処理がRecordTypeから遠すぎます。また、異なるプロセッサが理論的に実行できるさまざまな種類の処理があります。最後に、RecordTypeは、内部で定義されている最小限の状態を変えるメソッドを備えた単純なDTOクラスになるように設計されています。

解決

ビジターそのような場合には、通常、パターンが使用されます。コードはもう少し複雑ですが、新しいものを追加した後 RecordType サブクラスあなた した方が良い それ以外の場合はコンパイルされないため、どこにでもロジックを実装してください。と instanceof あちこちで、1つか2つの場所を見逃すのはとても簡単です。

例：

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 どちらのタイプが見つからない場合。

他のヒント

私のおすすめ：

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()
    {
        ...
    }
}

実行する必要があるコードがモデルが知らないもの（UIなど）に結合されている場合、一種のダブルディスパッチまたは訪問者パターンを使用する必要があります。

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

もう1つの可能なアプローチは、プロセス（）を作成することです（または、それを「dosubclassprocess（）」と呼ぶことが、そのことを明確にしている場合）抽象（記録型で）、そしてサブクラスに実際の実装を持っていることです。例えば

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

いくつかのタイプミスに注意してください - 私はそれらをすべて修正したと思います。

デザインは目的のための手段であり、目標や制約を知らないことで、その特定の状況であなたのデザインが良いのか、それがどのように改善されるのかは誰にもわかりません。

ただし、オブジェクト指向設計では、メソッドの実装を別のクラスに維持する標準的なアプローチは、各タイプの個別の実装を持っている間、訪問者パターン.

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

別の言い方をすれば、おそらく到達不可能なコードパスは、未定義の動作をもたらすのではなく、例外をスローする必要があります。

あなたの例のように、使用していないように、1つの考え方の悪いデザイン ビジター パターン、該当する場合。

もう一つは効率. instanceof 比較するなど、他の手法と比較して、非常に遅いです class 平等を使用したオブジェクト。

使用するとき ビジター パターン、通常は効果的でエレガントなソリューションが使用されています Map サポート間のマッピング用 class と ビジター 実例。大きい if ... else でブロックします instanceof チェックは非常に効果がありません。

ライセンス： CC-BY-SA と帰属

所属していません StackOverflow