4

多くの異なる .cpp ファイルで次のようなことを行う C++ プログラムがあります。

if (!thing1.empty() && !thing2.empty())
{
    if (thing1.property < thing2.property)
        return func1();
    else if (thing2.property < thing1.property)
        return func2();
    else
        return func3();
}
else if (!thing1.empty())
{
    return func1();
}
else if (!thing2.empty())
{
    return func2();
}
else
{
   return func4();
}

私は、thing1 が thing2 よりも大きい場合は一方向に func を実行しようとしていますが、逆の場合は逆方向に実行しようとしていますが、存在しない場合はその半分に対してのみ func を実行します。どちらも存在しない場合は、まったく別のことを行います。プロパティ、関数、および戻り値の型は、このパターンを使用するたびに異なります。このネストされた if ステートメントの醜い混乱よりも、私がやりたいことのためのより良い設計はありますか?

編集:私のコード例は単純化しすぎていることに気付きました。これは、うまくいけば問題をよりよく説明する私の実際のコードの一部です（ただし、はるかに面倒です）：

if (!diamondsOnly.empty() && !clubsOnly.empty())
{
    if (diamondsOnly.size() < clubsOnly.size())
    {
        if (passHighCards(player.hand, getHighCards(Card::DIAMONDS), result))
            return result;
        if (passHighCards(player.hand, getHighCards(Card::CLUBS), result))
            return result;
    }
    else if (clubsOnly.size() < diamondsOnly.size())
    {
        if (passHighCards(player.hand, getHighCards(Card::CLUBS), result))
            return result;
        if (passHighCards(player.hand, getHighCards(Card::DIAMONDS), result))
            return result;
    }
    else
    {
        if (diamondsOnly.back().value > clubsOnly.back().value)
        {
            if (passHighCards(player.hand, getHighCards(Card::DIAMONDS), result))
                return result;
            if (passHighCards(player.hand, getHighCards(Card::CLUBS), result))
                return result;
        }
        else
        {
            if (passHighCards(player.hand, getHighCards(Card::CLUBS), result))
                return result;
            if (passHighCards(player.hand, getHighCards(Card::DIAMONDS), result))
                return result;
        }
    }
}
else if (!diamondsOnly.empty())
{
    if (passHighCards(player.hand, getHighCards(Card::DIAMONDS), result))
        return result;
}
else if (!clubsOnly.empty())
{
    if (passHighCards(player.hand, getHighCards(Card::CLUBS), result))
        return result;
}
4

4 に答える 4

5

決めてから行動する

実際のコードを見ると、最初に気付くのは、定数だけが異なるほぼ同一の呼び出しがたくさんあるということです。複雑なロジックに設定されたパラメーターを使用して、1 か所で呼び出しを行います。

// Decide what to do.
std::vector<Card::Suit> passOrder;
if (!diamondsOnly.empty() && !clubsOnly.empty()) {
    // .. complicated logic that adds suits to passOrder ..
}

// Do it.
for (auto suit : passOrder) {  // This is C++11 style -- alter as needed
    if (passHighCards(player.hand, getHighCards(suit), result))
        return result;
}

(ベクトルが常に 1 つまたは 2 つだけの場合、ベクトルを使用するのはやり過ぎかもしれませんが、実際のコードはすべてのスーツを処理できると想定しています。)

これにより、読みやすくなります。プログラマーは、最初にカードを渡す順序を決定し、次に実際にカードを渡すことがわかります。2つの別々のステップがより明確になります。passCards を呼び出す場所が 1 つだけあると、決定ロジック全体にコピーを配置するよりも、愚かなタイプミスが発生しにくくなります。また、非常に特定のケースにブレークポイントを設定したり、ループの最初にブレークポイントを設定して passOrder を検査したりできるため、デバッグが容易になります。

ロジックを簡素化する

次に、決定ロジックを単純化します。

オプション:

  • センチネル: 場合によっては、コンテナーの 1 つの最後のカードを逆参照する必要があるため、複雑な問題が発生します。これは、コンテナーが空の場合には実行できません。空のケースをテストする必要がないように、コンテナーにセンチネルを追加することを検討する価値がある場合があります。空のケースが決してないことが保証されます。これは実行できる場合とできない場合があります。コンテナを扱う他のすべてのコードがセンチネルを理解できるようにする必要があります。

  • 例外のみ: デフォルトの順序 (例: ダイヤモンドの次にクラブ) を選択することで、いくつかの句を省略できます。次に、クラブの次にダイヤモンドが必要な場合のみをテストします。

  • テンポラリで表現する: 必要な比較を簡素化し、これらのテンポラリの観点から比較を表現する適切な名前のテンポラリを作成します。空/空ではないケースを一時的に除外すると、0 や -1 などの適切な SENTINEL_VALUE を選択することで、いくつかのケースを排除できることに注意してください。

すべてを一緒に入れて：

// For readability.
const bool fewerClubs = clubsOnly.size() < diamondsOnly.size();
const bool sameNumber = clubsOnly.size() == diamondsOnly.size();
const int lastDiamondValue =  diamondsOnly.empty() ? -1 : diamondsOnly.back().value;
const int lastClubValue    =  clubsOnly   .empty() ? -1 : clubsOnly   .back().value;

// Decide what order to select cards for passing.
std::vector<Card::Suit> passOrder;
passOrder.push_back(Cards::DIAMONDS);  // default order
passOrder.push_back(Cards::CLUBS);

// Do we need to change the order?
if (fewerClubs || (sameNumber && lastClubValue > lastDiamondValue)) {
    // Yep, so start with the clubs instead.
    passOrder[0] = Cards::CLUBS;
    passOrder[1] = Cards::DIAMONDS;
}

// Do it.
for (auto suit : passOrder) {  // This is C++11 style -- alter as needed
    if (passHighCards(player.hand, getHighCards(suit), result))
        return result;
}

これは、getHighCards が空の可能性のあるコンテナーを入力として処理することを前提としています。

于 2012-05-10T17:26:16.120 に答える
4

それが大幅な改善であるかどうかはわかりませんが、次の方法でふさふさを少し元に戻すことができます。

if (thing1.empty() && thing2.empty())
   return func4();
else if (thing1.empty())
    return func2();
else if (thing2.empty())
    return func1();
else if (thing1.property < thing2.property)
    return func1();
else if (thing2.property < thing1.property)
    return func2();
else
    return func3();

一貫性を保つために中括弧を削除しました。それらは元に戻すことができますが、読みやすさの利点があったとしてもごくわずかで、コードの長さを増やします。これにより、ネガも回避されます。それらは常に条件を（少し）読みにくくします。それはあなたのコードでは大きな問題ではありませんでした。条件が複雑な場合もあります。

returnすべてのアクションはステートメントであるため、else毎回削除される可能性があると正当に主張することができます。

より大きな例を考えると、状況に応じて、すべてのコードが1つまたは2つの非常によく似たアクションにつながります。

このような状況では、カーニハンとプローガによる優れた（しかし少し古くて絶版の）本「プログラム書法の要素」の口述の1つを適用する必要があります。

  • サブルーチン呼び出しにより、引数リストの不規則性を要約することができます[...]
  • [t]サブルーチン自体がコードの規則性を要約します[...]

それに応じてコーディングし、以前に提案されたのと同様の方法で条件ツリーのふさふさを回避します。

CardType pass[2] = { -1, -1 };  // Card::INVALID would be nice

if (clubsOnly.empty() && diamondsOnly.empty())
{
    ...do undocumented action for no diamonds or clubs...
}
else if (diamondsOnly.empty())
{
    pass[0] = Card::CLUBS;
}
else if (clubsOnly.empty())
{
    pass[0] = Card::DIAMONDS;
}
else if (diamondsOnly.size() < clubsOnly.size())
{
    pass[0] = Card::DIAMONDS;
    pass[1] = Card::CLUBS;
}
else if (clubsOnly.size() < diamondsOnly.size())
{
    pass[0] = Card::CLUBS;
    pass[1] = Card::DIAMONDS;
}
else if (diamondsOnly.back().value > clubsOnly.back().value)
{
    pass[0] = Card::DIAMONDS;
    pass[1] = Card::CLUBS;
}
else
{
    pass[0] = Card::CLUBS;
    pass[1] = Card::DIAMONDS;
}

次に、すべての条件をカバーしたら、単純なループを実行して適切な処理を実行します。

for (int i = 0; i < 2; i++)
{
    if (pass[i] != -1 && passHighCards(player.hand, getHighCards(pass[i]), result))
        return result;
}

...undocumented what happens here...

2はやや不快です。2回表示されます。

ただし、全体として、各テストの後に単純な対称アクションを使用した一連の線形テストが提供されます（アクションは1行以上の長さであるため、一貫性を保つために、今回は中括弧を保持します。一貫性は、中括弧自体の有無よりも重要です。 ）。何をすべきかについての決定が完了したら、実際に行ってそれを行います。

于 2012-05-10T14:35:32.510 に答える
3

すべての条件を計算し、決定を行う関数に送信して、次に何をすべきかを示すコードを返すことができます。繰り返されるすべての「パターン」は、関数内にあります。

// return decision1 or decision2, depending on the result of comparison of properties
// Note: type is ssize_t to accommodate bool, size_t and whatever type is 'value'
int decision(ssize_t property1, ssize_t property2, int decision1, int decision2)
{
    if (property1 > property2)
        return decision1;
    else if (property2 > property1)
        return decision2;
    else
        return 0;
}

some_func()
{
    int decision = decide(!diamondsOnly.empty(), !clubsOnly.empty(), 1, 2);
    if (!decision)
        decision = decide(diamondsOnly.size(), clubsOnly.size(), 3, 4);
    if (!decision)
        decision = decide(diamondsOnly.back().value, clubsOnly.back().value, 3, 4);

    bool flag;
    switch (decision)
    {
        case 1:
            flag = passHighCards(player.hand, getHighCards(Card::DIAMONDS), result);
            break;
        case 2:
            flag = passHighCards(player.hand, getHighCards(Card::CLUBS), result);
            break;
        case 3:
            flag = passHighCards(player.hand, getHighCards(Card::DIAMONDS), result);
            flag = passHighCards(player.hand, getHighCards(Card::CLUBS), result);
            break;
        case 4:
            flag = passHighCards(player.hand, getHighCards(Card::CLUBS), result);
            flag = passHighCards(player.hand, getHighCards(Card::DIAMONDS), result);
            break;
        default:
            flag = whatever();
            break;
    }

    if (flag)
        return result;
}

上記のコードでは、switchステートメントは DRY に違反しています。これがまだ問題であると思われる場合は、個々のビットでアクションをエンコードする「賢い」コードを使用できます。

  • ビット 0: 何かをするかどうか
  • ビット 1: 最初に何をすべきか
  • ビット 2: 2 つ目の処理を行うかどうか
  • ビット 3: 次に何をすべきか
if ((decision & 1) == 0) {flag = whatever;}
else
{
    thing1 = (decision & 2) ? Card::DIAMONDS : Card::CLUBS
    flag = passHighCards(player.hand, thing1, result);
    if (decision & 4)
    {
        thing2 = (decision & 8) ? Card::DIAMONDS : Card::CLUBS;
        flag = passHighCards(player.hand, thing2, result);
    }
}

しかし、私の意見では、この「DRY 準拠」の作品は、switch.

于 2012-05-10T16:49:16.123 に答える
1

IMyCompare などのインターフェイスを作成します。IMyCompare を受け取り、Func を返すメソッドを使用すると、それぞれに全体を実装できます。

AThing.MyCompare(otherThing); のようなことをします。

どの関数の各条件が物事の型によって固定されていない場合は、関数の配列を比較呼び出しに渡します。

MyCompare から int を返すだけで誘惑され、それを使用する方法を私が思う別のものに委譲します。

于 2012-05-10T14:46:07.213 に答える