6

あまりにも多くのパラメーターを渡す次の悪いコードをどのように修正しますか?

void helper1(int p1, int p3, int p5, int p7, int p9, int p10) {
  // ...
}

void helper2(int p1, int p2, int p3, int p5, int p6, int p7, int p9, int p10) {
  // ...
}

void foo(int p1, int p2, int p3, int p4, int p5, int p6, int p7, int p8, 
         int p9, int p10) {
  helper1(p1, p3, p5, p7, p9, p10);
  helper2(p1, p2, p3, p5, p6, p7, p9, p10);
}

私は2つの異なるアプローチを見ています：

アプローチ 1: すべての関数をクラスに入れる

class Foo {
  private:
    int p1, p2, p3, p4, p5, p6, p7, p8, p9, p10;
    void helper1() {}
    void helper2() {}
  public:
    void foo() {
      helper1();
      helper2();
    }
    // add constructor
};

アプローチ 2: パラメータをクラスとして渡すだけ

struct FooOptions {
    int p1, p2, p3, p4, p5, p6, p7, p8, p9, p10;
};

void helper1(const FooOptions& opt) {
  // ...
}

void helper2(const FooOptions& opt) {
  // ...
}

void foo(const FooOptions& opt) {
  helper1(opt);
  helper2(opt);
}

アプローチの長所と短所は何ですか?

アプローチ 1 の利点は、helper関数を仮想化すると、それらをサブクラス化してオーバーロードできるため、柔軟性が増すことです。しかし、私の場合 (私が示したおもちゃのミニの例を除いて)、そのようなヘルパーはテンプレート化されることが多いため、とにかく仮想化することはできません。

アプローチ 2 の利点は、ヘルパー関数を他の関数からも簡単に呼び出すことができることです。

（この質問は関連していますが、これら2つの選択肢については説明していません。）

4

7 に答える 7

7

簡潔な答え：

あけましておめでとう！オプション #1 は避け、オプション #2 を使用するのは、関数から離れた意味のある明確で論理的なグループにパラメーターを分離できる場合のみです。

長い答え

あなたが同僚から説明したように、関数の例をたくさん見てきました。コードの匂いが悪いという事実については同意します。ただし、パラメーターを渡す必要がないようにパラメーターをクラスにグループ化し、これらのヘルパー関数に基づいて任意にグループ化することを決定すると、さらに悪臭が発生する可能性があります。読みやすさと理解を向上させているかどうかを自問する必要があります。

calcTime(int p1, int p2, int p3, int p4, int p5, int p6) {
    dist = calcDistance( p1, p2, p3 );
    speed = calcSpeed( p4, p5, p6 );
    return speed == 0 : 0 ? dist/speed; }

パラメータ間に明確な違いがあるため、より理解しやすいように物事をグループ化できます。次に、アプローチ＃2をお勧めします。

一方、私が渡されたコードは次のようになります。

calcTime(int p1, int p2, int p3, int p4, int p5, int p6) {
    height = height( p1, p2, p3, p6 );
    type = getType( p1, p4, p5, p6 );
    if( type == 4 ) {
        return 2.345; //some magic number
    }
    value = calcValue( p2, p3, type ); //what a nice variable name...
    a = resetA( p3, height, value );
    return a * value; }

これにより、これらのパラメーターは、クラスごとに意味のあるものに分割するのに必ずしも適していないという感覚が残ります。代わりに、次のようなものをひっくり返して提供する方がよいでしょう

calcTime(Type type, int height, int value, int p2, int p3)

そしてそれを呼び出す

calcTime(
    getType( p1, p4, p5, p6 ),
    height( p1, p2, p3, p6 ),
    p3,
    p4 );

頭の中の小さな声が「DRY、DRY、DRY！」と叫ぶので、背筋が震えます。どちらがより読みやすく、したがって保守しやすいですか?

誰かがパラメーターの 1 つを設定するのを忘れる可能性が非常に高いため、オプション #1 は頭の中で実行できません。これは、単純な単体テストに合格する検出困難なバグに非常に簡単につながる可能性があります。YMMV。

于 2010-01-01T17:12:17.977 に答える
2

クラスと構造体を書き直して、次のように分析できるようにしました。 

class Point {
      private:
        int x, y
        void moveUp() {}
        void moveRight() {}
      public:
        void moveUpAndRight() {
          moveUp();
          moveRight();
        }
        // add constructor
    };

struct Point {
  int x, y;
};

void moveUp {}
void moveRight {}
void moveUpAndRight {
  moveUp {}
  moveRight {}
}

クラスを使用すると、オブジェクトを操作できます。ポイント オブジェクトを使用するコードは、標準インターフェイスを使用してポイントを移動します。座標系が変更され、その座標系を検出するようにポイント クラスが変更された場合、これにより、同じインターフェイスを複数の座標系で使用できるようになります (何も壊れません)。この例では、アプローチ 1 を使用するのが理にかなっています。

組織の目的でアクセスする関連データをグループ化するだけの場合...

struct Dwarves {
  int Doc, Grumpy, Happy, Sleepy, Bashful, Sneezy, Dopey;
}

void killDwarves(Dwarves& dwarves) {}
void buryDwarves(Dwarves& dwarves) {}

void disposeOfDwarves(Dwarves& dwarves) {
  killDwarves(dwarves);
  buryDwarves(dwarves);
}

次に、アプローチ 2 を使用するのが理にかなっています。

これは OO 設計の選択であるため、複数の正しい解決策があり、与えられたものから決定的な答えを出すことは困難です。

于 2010-01-01T16:30:43.660 に答える
1

p1、p2などが実際には単なるオプション（フラグなど）であり、互いに完全に無関係である場合は、2番目のオプションを使用します。

それらのいくつかが常に一緒になる傾向がある場合は、ここに表示されるのを待っているクラスがいくつかある可能性があります。最初のオプションを選択しますが、おそらく1つの大きなクラスではありません（実際には複数のクラスを作成する必要があります-難しいです実際のパラメータの名前なしで言う）。 

class Something {
   int p1, p3, p5 ...;
   void helper1a();
};

class SomethingElse {
   int p7, p9, p10 ...;
   void helper1b();
};

class Options {
   int p2, p4, ...
};

void foo(Something &s, SomethingElse & else, Options &options) {
  helper2(options);
  s.helper1a();
  else.helper1b();
}
于 2010-01-01T16:03:49.593 に答える
1

クラスを純粋なデータストレージとして使用したり、フィールドを使用してデータをメソッドに渡したりしないでください。通常、メソッドが必要とするパラメーターが多すぎる場合は、必要なパラメーターが多すぎるため、メソッドから機能を個別のメソッドに抽出する必要があります。おそらく、メソッドのより具体的な例を提供して、より大きな視点からそれを議論できるようにすることができます。

于 2010-01-01T19:09:15.570 に答える
1

それらをクラスに入れるアプローチは、正しく「感じられません」。古いコードを改造して、そうではないもののように見せようとする試みのようです。時間が経つにつれて、「オブジェクト指向」スタイルのコードに移行することは可能かもしれませんが、最終的には 2 つのパラダイムの不適切な組み合わせになる可能性が高いようです。しかし、それは主に、私が扱っているオブジェクト指向以外のコードで何が起こるかについての私の考えに基づいています。

したがって、これら 2 つのオプションの間では、struct オプションの方が少し優れていると思います。これにより、別の関数がパラメーターを構造体にパッケージ化し、ヘルパー関数の 1 つを呼び出すことができる柔軟性が残されます。しかし、この方法は、自己文書化の側面に問題があるように思えます。他の場所から Helper1 への呼び出しを追加する必要が生じた場合、どのパラメーターを渡す必要があるかは明確ではありません。

したがって、多くの反対票が寄せられているように感じますが、変更しないことが最善の方法だと思います。関数の 1 つに新しい呼び出しを入力すると、優れたエディターがパラメーター リストを表示するという事実により、それを正しく行うことが非常に簡単になります。また、交通量の多いエリアでない限り、コストはそれほど高くありません。また、64 ビット環境では、より多くのパラメーター (4 つですか?) が通常レジスターで送信されるため、さらに少なくなります。

これについては、多くのレスポンダーが参加している関連する質問があります。

于 2010-01-01T16:30:24.067 に答える
1

知らない。それは本当にパラメータが何であるかに依存します。呼び出すことができる魔法の「20 個の関数パラメーターを非表示にする」設計パターンはありません。あなたができる最善のことは、コードをリファクタリングすることです。パラメーターが何を表しているかによって、それらのいくつかをパラメーター クラスにグループ化するか、すべてを 1 つのモノリシック クラスに入れるか、全体を複​​数のクラスに分割することが重要な場合があります。機能する可能性のあるオプションの 1 つは、何らかの形式のカリー化です。一度にオブジェクトを少しずつ渡し、そのたびに次の呼び出しを呼び出すことができる新しいオブジェクトを生成し、次のパラメーターのバッチを渡します。これは、一部のパラメーターが呼び出し間で同じままであることが多い場合に特に意味があります。

これのより単純な変形は、一部のパラメーターがコンストラクターで渡されるクラスですべてをラップすることです (通常、呼び出しのバッチ全体で同じままであるパラメーター)。その他のパラメーターは、呼び出しごとに提供する必要があります。それらは常に変化しているため、実際の関数呼び出しで渡されます (おそらくオーバーロードされたoperator())。

コードの実際の意味を知らなければ、最適なリファクタリング方法を言うことはできません。

于 2010-01-01T18:31:14.133 に答える
1

オプション #2。

ただし、ドメインについてよく考えてみると、パラメーター値の意味のあるグループ化があり、ドメイン内のエンティティにマップできることがわかるでしょう。

次に、このオブジェクトのインスタンスを作成し、プログラムに渡すことができます。

通常、これは Application オブジェクトや Session オブジェクトなどのようなものです。

于 2010-01-01T17:21:54.880 に答える