2

私は昨日、Barから継承したクラスFooでこれを書きました：

public override void AddItem(double a, int b)
{
    //Code smell?
    throw new NotImplementedException("This method not usable for Foo items");
}

その後、これがバーを継承するのではなく、バーを使用する必要があることを示している可能性があるかどうか疑問に思いました。

継承と構成のどちらかを選択するのに役立つ他の「コードの臭い」は何ですか？

編集私はこれがスニペットであることを追加する必要があります、一般的な他の方法があります、私はあまり詳細に立ち入りたくありませんでした。私は作曲への切り替えの影響を分析する必要があり、バランスを崩すのに役立つ可能性のある他の「コードの臭い」があるのではないかと考えました。

4

8 に答える 8

18

上記の例は明らかにコードの匂いです。AddItemメソッドは基本クラスの動作ですBar。Fooが動作をサポートしていない場合はAddItem、から継承すべきではありませんBar。

より現実的な (C++) 例を考えてみましょう。次のクラスがあるとします。

class Animal
{
    void Breathe() const=0;
}

class Dog : public Animal
{
    // Code smell
    void Breathe() { throw new NotSupportedException(); }
}

動物は生き残るために呼吸をしなければならないため、基本抽象クラスAnimalは純粋な仮想メソッドを提供します。Breathe()呼吸をしなければ、定義上、動物ではありません。

Dogから継承するAnimalが動作をサポートしない新しいクラスを作成することBreathe()により、クラスによって規定された契約に違反していますAnimal。かわいそうな犬は生き残れません！

パブリック継承の単純なルールは、派生クラス オブジェクトが真に基底クラス オブジェクトである場合にのみ実行する必要があるということです。

あなたの特定の例では：

  • Foo契約AddItem()で規定された動作をサポートしていません。Bar
  • したがって、定義上、Fooは "not a"Barであり、継承すべきではありません。
于 2009-04-09T06:22:31.527 に答える
3

では、拡張機能のないFooがBarのように動作しない場合、なぜBarから継承するのでしょうか。考えてみると、基本クラスで「PASSWORD」のようなメソッドを仮想として宣言することすらしません。

于 2009-04-09T06:20:25.057 に答える
3

はい、メソッドを「実装しない」必要があるということは、おそらく「is-a」関係を使用すべきではないことを示しています。あなたの Foos は実際には Bars ではないようです。

しかし、あなたのFoos and Barsについて考えることから始めましょう. フーズバーですか？セットとサブセットを紙に描くことができますか? すべての Foo (つまり、Foo クラスのすべてのメンバー) も Bar (つまり、Bar クラスのメンバー) になりますか? そうでない場合は、おそらく継承を使用したくないでしょう。

Foo が実際には Bar ではなく、Foo が Bar を継承すべきではないことを示すもう 1 つのコード臭は、ポリモーフィズムを使用できないことです。Bar を引数として取るメソッドがあるとしますが、Foo を処理することはできません。(おそらく、引数で AddItem メソッドを呼び出すためです!) チェックを追加するか、NotImplementedException を処理する必要があるため、コードが複雑になり、理解しにくくなります。（そして臭い！）

于 2009-04-09T06:39:04.870 に答える
1

上記の例はおそらく何かがうまくいかないことを示していますが、NotImplementedException 自体は常に間違っているわけではありません。スーパークラスのコントラクトと、このコントラクトを実装するサブクラスがすべてです。スーパークラスにそのようなメソッドがある場合

// This method is optional and may be not supported
// If not supported it should throw NotImplementedException 
// To find out if it is supported or not, use isAddItemSupported()
public void AddItem(double a, int b){...}

次に、このメソッドをサポートしていなくても、コントラクトでは問題ありません。サポートされていない場合は、おそらく UI で対応するアクションを無効にする必要があります。したがって、そのような契約に問題がなければ、サブクラスはそれを正しく実装します。

クライアントがすべてのクラス メソッドを使用するわけではなく、決して使用しないことを明示的に宣言する場合の別のオプション。このような

// This method never modifies orders, it just iterates over 
// them and gets elements by index.
// We decided to be not very bureaucratic and not define ReadonlyList interface,
// and this is closed-source 3rd party library so you can not modify it yourself.
// ha-ha-ha
public void saveOrders(List<Order> orders){...}

次に、追加、削除、およびその他のミューテーターをサポートしない List の実装をそこに渡しても問題ありません。それを文書化するだけです。

// Use with care - this implementation does not implement entire List contract
// It does not support methods that modify the content of the list.
public ReadonlyListImpl implements List{...}

コードですべてのコントラクトを定義するのは良いことですが、コントラクトに違反しているかどうかをコンパイラに確認させるため、合理的ではない場合があり、コメントのように定義の弱いコントラクトに頼らなければならないことがあります。

簡単に言えば、スーパークラスがコードだけではないコントラクトによって定義されていることを考慮して、サブクラスをスーパークラスとして本当に安全に使用できるかどうかという問題になります。

于 2009-04-16T21:04:12.787 に答える
1

もちろん、直接の逆もあります。Foo が所有する Bar にメッセージを転送するだけの多くのインターフェイスを実装している場合、これはそれが Bar であることを示している可能性があります。ただし、においが常に正しいとは限りません。

于 2009-04-09T06:21:19.453 に答える
0

構成は複雑さを軽減するため、継承よりも望ましい方法です。より良いアプローチは、コンストラクター注入を使用し、Foo クラスで Bar への参照を保持することです。

于 2009-04-09T18:37:27.380 に答える