C ++での実装の継承。本当の話

こんにちは、Habr！



インスピレーション、C ++のトピックに関する出版社のポートフォリオを補充する方法を探して、私たちはArthur O'Dwyerのブログに出くわしました。彼は、ちなみに、どこからともなく登場したC ++に関する1冊の本をすでに書いていました。今日の投稿はクリーンなコードについてです。事件自体と作者の両方があなたの興味を引くことを願っています。



古典的な多態性コードを扱うほど、それが成長した「現代の」イディオム、つまり非仮想インターフェースのイディオム、リスコフ置換の原則、すべてのクラスは抽象的または最終的でなければならないというスコット・マイヤーズの規則、階層は厳密に2レベルである必要があり、基本クラスはインターフェイスの統一性を表し、実装を再利用しないというルールが必要です。



今日は、「実装の継承」がどのように問題を引き起こしたか、そしてそれを解明するために使用したパターンを示すコードを紹介したいと思います。残念ながら、私を動揺させたコードは非常に判読できなかったので、ここで私はそれを単純化された少し主題指向の形式で表示することにしました。問題全体の概要を段階的に説明します。

ステージ1：トランザクション

私たちの主題分野が「銀行取引」であるとしましょう。古典的な多形インターフェース階層があります。

class Txn { ... };
class DepositTxn : public Txn { ... };
class WithdrawalTxn : public Txn { ... };
class TransferTxn : public Txn { ... };

多種多様なトランザクションには特定の共通APIがあり、トランザクションのタイプごとに固有のAPIもあります。

class Txn {
public:
    AccountNumber account() const;
    std::string name_on_account() const;
    Money amount() const;
private:
    //  
};

class DepositTxn : public Txn {
public:
    std::string name_of_customer() const;
};

class TransferTxn : public Txn {
public:
    AccountNumber source_account() const;
};

ステージ2：トランザクションフィルター

しかし実際には、私たちのプログラムはトランザクションを実行しませんが、疑わしいトランザクションにフラグを立てるためにそれらを追跡します。人間のオペレーターは、「10,000ドルを超えるすべてのトランザクションにフラグを立てる」、「チェックリストWのユーザーに代わって実行されるすべてのトランザクションにフラグを立てる」など、特定の基準を満たすフィルターを設定できます。内部的には、さまざまなタイプのオペレーター構成可能フィルターを古典的な多形階層として表します。

class Filter { ... };
class AmountGtFilter : public Filter { ... };
class NameWatchlistFilter : public Filter { ... };
class AccountWatchlistFilter : public Filter { ... };
class DifferentCustomerFilter : public Filter { ... };
class AndFilter : public Filter { ... };
class OrFilter : public Filter { ... };
         API.
class Filter {
public:
    bool matches(const Txn& txn) const {
        return do_matches(txn);
    }
private:
    virtual bool do_matches(const Txn&) const = 0;
};

単純なフィルターの例を次に示します。

class AmountGtFilter : public Filter {
public:
    explicit AmountGtFilter(Money x) : amount_(x) { }
private:
    bool do_matches(const Txn& txn) const override {
        return txn.amount() > amount_;
    }

    Money amount_;
};

ステージ3：初めてつまずいた

一部のフィルターは、実際には特定のトランザクションに固有のAPIにアクセスしようとします。これらのAPIについては上記で説明しました。DifferentCustomerFilter実行者の名前が請求書に指定されている名前と異なるトランザクションにタグを付けようとしているとしましょう。例として、銀行が厳しく規制されていると仮定しましょう。このアカウントの所有者だけがアカウントからお金を引き出すことができます。したがって、DepositTxnトランザクションを行ったクライアントの名前を記録することに関心があるのはクラスだけです。

class DifferentCustomerFilter : public Filter {
    bool do_matches(const Txn& txn) const override {
        if (auto *dtxn = dynamic_cast<const DepositTxn*>(&txn)) {
            return dtxn->name_of_customer() != dtxn->name_on_account();
        } else {
            return false;
        }
    }
};

これはdynamic_castの古典的な乱用です！（クラシック-常に見つかっているため）。このコードを修正するには、「Classically polymorphicvisit」（2020-09-29）の方法を適用しようとしますが、残念ながら、改善は見られませんでした。

class DifferentCustomerFilter : public Filter {
    bool do_matches(const Txn& txn) const override {
        my::visit<DepositTxn>(txn, [](const auto& dtxn) {
            return dtxn.name_of_customer() != dtxn.name_on_account();
        }, [](const auto&) {
            return false;
        });
    }
};

したがって、コードの作成者はアイデアを思いついた（sarcasm！）。いくつかのフィルターにケース感度を実装しましょう。基本クラスを次のFilterように書き直してみましょう。

class Filter {
public:
    bool matches(const Txn& txn) const {
        return my::visit<DepositTxn, WithdrawalTxn, TransferTxn>(txn, [](const auto& txn) {
            return do_generic(txn) && do_casewise(txn);
        });
    }
private:
    virtual bool do_generic(const Txn&) const { return true; }
    virtual bool do_casewise(const DepositTxn&) const { return true; }
    virtual bool do_casewise(const WithdrawalTxn&) const { return true; }
    virtual bool do_casewise(const TransferTxn&) const { return true; }
};

class LargeAmountFilter : public Filter {
    bool do_generic(const Txn& txn) const override {
        return txn.amount() > Money::from_dollars(10'000);
    }
};

class DifferentCustomerFilter : public Filter {
    bool do_casewise(const DepositTxn& dtxn) const override {
        return dtxn.name_of_customer() != dtxn.name_on_account();
    }
    bool do_casewise(const WithdrawalTxn&) const override { return false; }
    bool do_casewise(const TransferTxn&) const override { return false; }
};

この巧妙な戦術は、あなたが書く必要のあるコードの量を減らしますDifferentCustomerFilter。しかし、私たちはOOPの原則の1つ、つまり実装の継承の禁止に違反しています。関数はFilter::do_generic(const Txn&)純粋でも最終でもありません。これは私たちを悩ませるために戻ってきます。



ステップ4：もう一度つまずいた



アカウント所有者が州のブラックリストに含まれているかどうかを確認するフィルターを作成しましょう。これらのリストのいくつかをテストしたいと思います。

class NameWatchlistFilter : public Filter {
protected:
    bool is_flagged(std::string_view name) const {
        for (const auto& list : watchlists_) {
            if (std::find(list.begin(), list.end(), name) != list.end()) {
                return true;
            }
        }
        return false;
    }

private:
    bool do_generic(const Txn& txn) const override {
        return is_flagged(txn.name_on_account());
    }

    std::vector<std::list<std::string>> watchlists_;
};

ああ、より広いグリッドを描画する別のフィルターを作成する必要があります-それはアカウント所有者とユーザー名の両方をチェックします。繰り返しますが、元のコードの作成者は（皮肉！）賢い考えを持っています。なぜロジックを複製するのかis_flagged、それをよりよく継承しましょう。継承はコードの再利用ですよね？

class WideNetFilter : public NameWatchlistFilter {
    bool do_generic(const Txn& txn) const override {
        return true;
    }
    bool do_casewise(const DepositTxn& txn) const override {
        return is_flagged(txn.name_on_account()) || is_flagged(txn.name_of_customer());
    }
    bool do_casewise(const WithdrawalTxn& txn) const override {
        return is_flagged(txn.name_on_account());
    }
    bool do_casewise(const TransferTxn& txn) const override {
        return is_flagged(txn.name_on_account());
    }
};

結果のアーキテクチャがひどく混乱していることに注意してください。NameWatchlistFilterオーバーライドdo_genericのみ口座名義の姓を検証するためには、その後、WideNetFilter元のビューにそれを上書きします。（WideNetFilter再定義しなかった場合、マークされていないがマークされWideNetFilterているデポジットトランザクションでは正しく機能しませname_on_account()んでしたname_of_customer()。）これは紛らわしいですが、今のところは機能します。

ステージ5：一連の不快なイベント

この技術的な負債は、新しいタイプのトランザクションを追加する必要があったため、予期しない方向に私たちを苦しめました。銀行自体が開始する手数料と利息の支払いを表すクラスを作成しましょう。

class FeeTxn : public Txn { ... };

class Filter {
public:
    bool matches(const Txn& txn) const {
        return my::visit<DepositTxn, WithdrawalTxn, TransferTxn, FeeTxn>(txn, [](const auto& txn) {
            return do_generic(txn) && do_casewise(txn);
        });
    }
private:
    virtual bool do_generic(const Txn&) const { return true; }
    virtual bool do_casewise(const DepositTxn&) const { return true; }
    virtual bool do_casewise(const WithdrawalTxn&) const { return true; }
    virtual bool do_casewise(const TransferTxn&) const { return true; }
    virtual bool do_casewise(const FeeTxn&) const { return true; }
};

最も重要なステップ：更新するのを忘れWideNetFilter、のオーバーライドを追加しましたWideNetFilter::do_casewise(const FeeTxn&) const。この「おもちゃ」の例では、このようなエラーはすぐに許されないように見えるかもしれませんが、実際のコードでは、ある再定義者から別の数十行のコードまで、非仮想インターフェイスのイディオムはそれほど嫉妬深く観察されていません-この class WideNetFilter : public NameWatchlistFilterようにオーバーライドするものを見つけるのは難しくないと思いますdo_genericそしてdo_casewise、「ああ、ここで何かが混乱している。私は後でこれに戻ります」（そしてこれに戻ることは決してありません）。



いずれにせよ、この時点で私たちにはモンスターがいることをすでに確信していることを願っています。どうやって彼を魅了しますか？



リファクタリング、実装の継承を取り除きます。ステップ1



で実装の継承を取り除くにはFilter::do_casewise、仮想メソッドは純粋または（論理的に）最終的でなければならないというスコットマイヤーズの仮定を適用します。この場合、階層を浅くする必要があるというルールに違反しているため、これは妥協点です。中級クラスを紹介します。

class Filter {
public:
    bool matches(const Txn& txn) const {
        return do_generic(txn);
    }
private:
    virtual bool do_generic(const Txn&) const = 0;
};

class CasewiseFilter : public Filter {
    bool do_generic(const Txn&) const final {
        return my::visit<DepositTxn, WithdrawalTxn, TransferTxn>(txn, [](const auto& txn) {
            return do_casewise(txn);
        });
    }

    virtual bool do_casewise(const DepositTxn&) const = 0;
    virtual bool do_casewise(const WithdrawalTxn&) const = 0;
    virtual bool do_casewise(const TransferTxn&) const = 0;
};

可能なすべてのトランザクションに大文字と小文字を区別する処理を提供するフィルターは、から単純に継承できるようになりましたCasewiseFilter。実装が汎用であるフィルターは、引き続きから直接継承しFilterます。

class LargeAmountFilter : public Filter {
    bool do_generic(const Txn& txn) const override {
        return txn.amount() > Money::from_dollars(10'000);
    }
};

class DifferentCustomerFilter : public CasewiseFilter {
    bool do_casewise(const DepositTxn& dtxn) const override {
        return dtxn.name_of_customer() != dtxn.name_on_account();
    }
    bool do_casewise(const WithdrawalTxn&) const override { return false; }
    bool do_casewise(const TransferTxn&) const override { return false; }
};

誰かが新しいトランザクションタイプを追加しCasewiseFilter、新しいオーバーロードを含めるように変更すると、抽象クラスになっdo_casewiseていることがわかりDifferentCustomerFilterます。新しいトランザクションタイプを処理する必要があります。これで、コンパイラは、エラーを静かに隠すために使用されていたアーキテクチャのルールに準拠するのに役立ちます。



またWideNetFilter、用語で定義することは現在不可能であることに注意してくださいNameWatchlistFilter。

リファクタリング、実装の継承を取り除きます。ステップ2

の実装継承を取り除くには、単独の責任WideNetFilterの原則が適用されます。現在、彼はNameWatchlistFilter2つの問題を解決しています。それは、本格的なフィルターとして機能し、能力を備えていis_flaggedます。これはフィルターではなく、便利なヘルパークラスであるため、APIに準拠する必要のないis_flaggedスタンドアロンクラスにWatchlistGroupしましょうclass Filter。

class WatchlistGroup {
public:
    bool is_flagged(std::string_view name) const {
        for (const auto& list : watchlists_) {
            if (std::find(list.begin(), list.end(), name) != list.end()) {
                return true;
            }
        }
        return false;
    }
private:
    std::vector<std::list<std::string>> watchlists_;
};

を継承せずにWideNetFilter使用できる ようになりました。どちらのフィルターでも、コンポジションはコードを再利用するためのツールですが、継承はそうではないため、よく知られた推奨事項に従い、継承よりもコンポジションを優先できます。is_flaggedNameWatchlistFilter

class NameWatchlistFilter : public Filter {
    bool do_generic(const Txn& txn) const override {
        return wg_.is_flagged(txn.name_on_account());
    }

    WatchlistGroup wg_;
};

class WideNetFilter : public CasewiseFilter {
    bool do_casewise(const DepositTxn& txn) const override {
        return wg_.is_flagged(txn.name_on_account()) || wg_.is_flagged(txn.name_of_customer());
    }
    bool do_casewise(const WithdrawalTxn& txn) const override {
        return wg_.is_flagged(txn.name_on_account());
    }
    bool do_casewise(const TransferTxn& txn) const override {
        return wg_.is_flagged(txn.name_on_account());
    }

    WatchlistGroup wg_;
};

繰り返しますが、誰かが新しいタイプのトランザクションを追加しCasewiseFilter、新しいオーバーロードを含めるように変更した場合、do_casewise必ずWideNetFilter抽象クラスになります。で新しいタイプのトランザクションを処理する必要がありますWideNetFilter（ただしではありませんNameWatchlistFilter）。コンパイラがやることリストを保持しているようなものです。

結論

この例は、使用する必要のあるコードと比較して、匿名化して非常に単純化しています。しかし、クラス階層の一般的な概要はdo_generic(txn) && do_casewise(txn)、元のコードの薄っぺらなロジックと同様に、まさにそれでした。上記のことから、バグが古い構造にどれほど気付かずに隠されていたかを理解するのはそれほど簡単ではないと思いますFeeTxn。この簡略化されたバージョンをあなたに提示したので（文字通りあなたのためにそれを噛み砕きました！）、私自身は実際に驚いています、コードの元のバージョンはとても悪かったですか？多分そうではありません...結局のところ、このコードはしばらくの間機能しました。

しかし、リファクタリングバージョンを使用してCasewiseFilter、特にWatchlistGroupはるかに優れていることが判明したことに同意していただければ幸いです。これらの2つのコードベースのどちらを使用するかを選択する必要がある場合は、2番目のコードベースを使用することを躊躇しません。