Clever Geek Handbook

Implementation inheritance in C ++. Real story

Hello, Habr!



In search of inspiration, how to replenish the portfolio of the publishing house on the topic of C ++, we came across the blog of Arthur O'Dwyer, who, by the way, had already written one book on C ++, which appeared out of nowhere . Today's post is about clean code . We hope that both the case itself and the author will be of interest to you.



The more I deal with classical polymorphic code, the more I appreciate the "modern" idioms that it has grown over - the idiom of a non-virtual interface, the Liskov substitution principle , Scott Myers' rule that all classes must be either abstract or final, the rule that any the hierarchy must be strictly two-level, and the rule that base classes express the unity of the interface , and do not reuse the implementation.



Today I want to show you a code snippet that demonstrates how "implementation inheritance" led to problems and what patterns I used to unravel it. Unfortunately, the code that upset me was so illegible that here I decided to show it in a simplified and slightly domain-specific form. I will try to outline the whole problem in stages.



Stage 1: Transactions



Let's say our subject area is "banking transactions." We have a classic polymorphic interface hierarchy. 



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


A wide variety of transactions have certain common APIs, and each type of transaction also has its own specific 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;
};


Stage 2: Transaction Filters



But in reality, our program does not execute transactions, but tracks them in order to flag suspicious transactions. The human operator can set filters that meet certain criteria, such as “flag all transactions over $ 10,000” or “flag all transactions performed on behalf of people on checklist W”. Internally, we represent the different types of operator-configurable filters as a classic polymorphic hierarchy. 



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


Here's an example of a simple filter: 



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


Stage 3: Stumbled for the first time



It turns out that some filters actually try to access those APIs that are specific to specific transactions - these APIs were discussed above. Let's say it DifferentCustomerFiltertries to tag any transaction where its executor name is different from the name specified in the invoice. For the sake of example, let's say that the bank is strictly regulated: only the owner of this account can withdraw money from an account. Therefore, only the class is DepositTxnconcerned about recording the name of the client who made the transaction. 



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


This is a classic abuse of dynamic_cast! (Classical - because it is found all the time). To fix this code, one could try to apply the method from “ Classically polymorphic visit ” (2020-09-29), but, unfortunately, no improvements have been observed: 



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


Therefore, the author of the code dawned on (sarcasm!) An idea. Let's implement case sensitivity in some filters. Let's rewrite the base class Filterlike this: 



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


This clever tactic reduces the amount of code you need to write in DifferentCustomerFilter. But we are violating one of the principles of OOP, namely, the prohibition of inheritance of implementation. The function is Filter::do_generic(const Txn&)neither pure nor final. This will come back to haunt us.



Step 4: Stumbled a second time



Now let's make a filter that checks if the account holder is on any state blacklist. We want to test several of these lists. 



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


Oh, we need to make another filter that draws a wider grid - it will check both the account holder and the username. Again, the author of the original code has a (sarcasm!) Clever idea. Why duplicate logic is_flagged, let's better inherit it. Inheritance is just a reuse of code, right? 



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


Notice how the resulting architecture is terribly confused. NameWatchlistFilteroverrides do_genericto only validate the last name of the account holder, then WideNetFilteroverrides it back to its original view. (If I had WideNetFiltern't redefined it back, it WideNetFilterwould have triggered incorrectly for any deposit transaction where it is name_on_account()not marked, but name_of_customer()marked.) This is confusing, but it works ... for now.



Stage 5: A series of unpleasant events



This technical debt bit us in such an unexpected direction, as we needed to add a new type of transaction. Let's make a class to represent commissions and interest payments initiated by the bank itself. 



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


The most important step: we forgot to update WideNetFilter, add an override for WideNetFilter::do_casewise(const FeeTxn&) const. In this "toy" example, such an error may immediately seem unforgivable, but in real code, where from one redefiner to another dozens of lines of code, and the idiom of a non-virtual interface is not very jealously observed - I think it will not be difficult to find one class WideNetFilter : public NameWatchlistFilterthat overrides like do_genericthis and do_casewise, and think, “Oh, something is confusing here. I'll come back to this later ”(and never come back to this).



In any case, I hope you are already convinced that by this point we have a monster. How do we disenchant him now?



Refactoring, getting rid of implementation inheritance. Step 1



To get rid of implementation inheritance inFilter::do_casewise, let's apply Scott Myers' postulate that any virtual method must be either pure or (logically) final. In this case, this is a compromise, since here we are breaking the rule that hierarchies should be shallow. We introduce an intermediate class: 



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


Filters that provide case sensitive processing for every possible transaction can now simply inherit from CasewiseFilter. Filters whose implementations are generic still inherit directly from 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; }
};


If someone adds a new transaction type and changes CasewiseFilterto include a new overload do_casewise, then we will see that we have DifferentCustomerFilterbecome an abstract class: we have to deal with a transaction of a new type. Now the compiler helps to comply with the rules of our architecture, where it used to quietly hide our errors.



Also note that it is now impossible to define WideNetFilterin terms NameWatchlistFilter.



Refactoring, getting rid of implementation inheritance. Step 2



To get rid of implementation inheritance in WideNetFilter, the principle of sole responsibility applies . At the moment, he is NameWatchlistFiltersolving two problems: it works as a full-fledged filter and has the ability is_flagged. Let's make it is_flaggeda stand-alone class WatchlistGroupthat doesn't need to conform to the API class Filter, as it's not a filter, but just a useful helper class. 



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


WideNetFilterCan now use is_flaggedwithout inheriting NameWatchlistFilter. In both filters, you can follow the well-known recommendation and prefer composition over inheritance, since composition is a tool for reusing code, but inheritance is not. 



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


Again, if someone adds a new type of transaction and modifies CasewiseFilterto include a new overload do_casewise, then we will make sure to WideNetFilterbecome an abstract class: we have to deal with transactions of a new type in WideNetFilter(but not in NameWatchlistFilter). It's like the compiler keeps a to-do list for us!



conclusions



I have anonymized and extremely simplified this example compared to the code I had to work with. But the general outline of the class hierarchy was exactly that, as was the flimsy logic do_generic(txn) && do_casewise(txn)from the original code. I think that from the above it is not so easy to understand how imperceptibly the bug was hidden in the old structure FeeTxn. Now that I have presented this simplified version to you (literally chewed it up for you!), I myself am practically surprised myself, was the original version of the code so bad? Maybe not ... after all, this code worked for a while.

But I hope you will agree that the refactoring version using CasewiseFilterand especially WatchlistGroupturned out to be much better. If I had to choose which of these two codebases to work with, I would not hesitate to take the second one.


More articles:


All Articles