Clever Geek Handbook

Héritage d'implémentation en C ++. Histoire vraie

Bonjour, Habr!



En quête d'inspiration, comment reconstituer le portfolio de la maison d'édition sur le thème du C ++, nous sommes tombés sur le blog d' Arthur O'Dwyer, qui, d'ailleurs, a déjà écrit un livre sur C ++, sorti de nulle part . Le message d'aujourd'hui concerne le code propre . Nous espérons que le cas lui-même et l'auteur vous intéresseront.



Plus je m'occupe du code polymorphe classique, plus j'apprécie les idiomes «modernes» sur lesquels il s'est développé - l'idiome d'une interface non virtuelle, le principe de substitution de Liskov , la règle de Scott Myers selon laquelle toutes les classes doivent être abstraites ou définitives, la règle selon laquelle tout la hiérarchie doit être strictement à deux niveaux, et la règle selon laquelle les classes de base expriment l' unité de l'interface et ne réutilisent pas l'implémentation.



Aujourd'hui, je veux vous montrer un morceau de code qui montre comment "l'héritage d'implémentation" a conduit à des problèmes, et quels modèles j'ai utilisé pour le démêler. Malheureusement, le code qui me dérangeait était tellement illisible qu'ici j'ai décidé de le montrer sous une forme simplifiée et légèrement orientée sujet. Je vais essayer de décrire tout le problème par étapes.



Étape 1: Transactions



Disons que notre sujet est «les transactions bancaires». Nous avons une hiérarchie d'interface polymorphe classique. 



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


Une grande variété de transactions a certaines API communes, et chaque type de transaction a également sa propre API spécifique. 



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


Étape 2: Filtres de transaction



Mais en réalité, notre programme n'exécute pas les transactions, mais les suit pour signaler les transactions suspectes. L'opérateur humain peut définir des filtres qui répondent à certains critères, tels que «marquer toutes les transactions de plus de 10 000 $» ou «marquer toutes les transactions effectuées au nom des personnes sur la liste de contrôle W». En interne, nous représentons les différents types de filtres configurables par l'opérateur sous la forme d'une hiérarchie polymorphe classique. 



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


Voici un exemple de filtre simple: 



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


Étape 3: J'ai trébuché la première fois



Il s'avère que certains filtres essaient en fait d'accéder à ces API spécifiques à des transactions spécifiques - ces API ont été décrites ci-dessus. Disons qu'il DifferentCustomerFilteressaie de marquer toute transaction où son nom d'exécuteur est différent du nom spécifié dans la facture. À titre d'exemple, supposons que la banque soit strictement réglementée: seul le propriétaire de ce compte peut retirer de l'argent d'un compte. Par conséquent, seule la classe se DepositTxnpréoccupe de l'enregistrement du nom du client qui a effectué la 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;
        }
    }
};


C'est un abus classique de dynamic_cast! (Classique - car on le trouve tout le temps). Pour corriger ce code, on pourrait essayer d'appliquer la méthode de « Visite polymorphe classique » (2020-09-29), mais, malheureusement, aucune amélioration n'a été observée: 



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


Par conséquent, l'auteur du code a eu une idée (sarcasme!). Implémentons la sensibilité à la casse dans certains filtres. Réécrivons la classe de base Filtercomme ceci: 



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


Cette tactique intelligente réduit la quantité de code que vous devez écrire DifferentCustomerFilter. Mais nous violons l'un des principes de la POO, à savoir l'interdiction d'héritage de mise en œuvre. La fonction n'est Filter::do_generic(const Txn&)ni pure ni définitive. Cela reviendra nous hanter.



Étape 4: Trébuché une deuxième fois



Maintenant, faisons un filtre qui vérifie si le titulaire du compte est sur une liste noire d'état. Nous voulons tester plusieurs de ces listes. 



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, nous devons créer un autre filtre qui dessine une grille plus large - il vérifiera à la fois le titulaire du compte et le nom d'utilisateur. Encore une fois, l'auteur du code original a une idée (sarcasme!) Intelligente. Pourquoi dupliquer la logique is_flagged, mieux vaut en hériter. L'héritage n'est qu'une réutilisation de code, non? 



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


Remarquez comment l'architecture résultante est terriblement confuse. NameWatchlistFilterremplace do_genericpour valider uniquement le nom de famille du titulaire du compte, puis le WideNetFilterremplace à sa vue d'origine. (Si je WideNetFilterne l' avais pas redéfini, cela WideNetFiltern'aurait pas fonctionné correctement pour toute transaction de dépôt où il n'est name_on_account()pas marqué, mais name_of_customer()marqué.) C'est déroutant, mais cela fonctionne ... pour le moment.



Étape 5: Une série d'événements désagréables



Cette dette technique nous a mordus dans une direction si inattendue, car nous devions ajouter un nouveau type de transaction. Créons une classe pour représenter les commissions et les paiements d'intérêts initiés par la banque elle-même. 



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


L'étape la plus importante: nous avons oublié de mettre à jour WideNetFilter, ajouter un remplacement pour WideNetFilter::do_casewise(const FeeTxn&) const. Cet exemple «jouet» de cette erreur peut sembler immédiatement impardonnable, mais dans le code réel, où un pereopredelatelya à une autre des dizaines de lignes de code, et l'idiome de l'interface non virtuelle n'est pas appliqué avec trop de zèle - je pense, ne sera pas difficile à rencontrer class WideNetFilter : public NameWatchlistFilter, redéfinissant comment do_genericcela et do_casewise, et pensez: «Oh, quelque chose est déroutant ici. J'y reviendrai plus tard »(et je n'y reviendrai jamais).



En tout cas, j'espère que vous êtes déjà convaincu qu'à ce stade, nous avons un monstre. Comment le désenchanter maintenant?



Refactoring, suppression de l'héritage d'implémentation. Étape 1



Pour supprimer l'héritage d'implémentation dansFilter::do_casewise, appliquons le postulat de Scott Myers selon lequel toute méthode virtuelle doit être pure ou (logiquement) finale. Dans ce cas, il s'agit d'un compromis, car ici nous violons la règle selon laquelle les hiérarchies doivent être superficielles. Nous introduisons une classe intermédiaire: 



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


Les filtres qui fournissent un traitement sensible à la casse pour chaque transaction possible peuvent désormais simplement hériter de CasewiseFilter. Les filtres dont les implémentations sont génériques héritent toujours directement de 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; }
};


Si quelqu'un ajoute un nouveau type de transaction et change CasewiseFilterpour inclure une nouvelle surcharge do_casewise, alors nous verrons que nous sommes DifferentCustomerFilterdevenus une classe abstraite: nous devons traiter une transaction d'un nouveau type. Maintenant, le compilateur aide à se conformer aux règles de notre architecture, où il cachait tranquillement nos erreurs.



A noter également qu'il est désormais impossible de définir WideNetFilteren termes NameWatchlistFilter.



Refactoring, suppression de l'héritage d'implémentation. Étape 2



Pour se débarrasser de l'héritage de mise en œuvre dans WideNetFilter, le principe de la responsabilité exclusive s'applique . Pour le moment, il NameWatchlistFilterrésout deux problèmes: il travaille comme un filtre à part entière et en a la capacité is_flagged. Faisons-en is_flaggedune classe autonome WatchlistGroupqui n'a pas besoin de se conformer à l'API class Filter, car ce n'est pas un filtre, mais juste une classe d'assistance utile. 



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


WideNetFilterPeut maintenant utiliser is_flaggedsans hériter NameWatchlistFilter. Dans les deux filtres, vous pouvez suivre la recommandation bien connue et préférer la composition à l'héritage, car la composition est un outil de réutilisation du code, mais l'héritage ne l'est pas. 



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


Encore une fois, si quelqu'un ajoute un nouveau type de transaction et modifie CasewiseFilterpour inclure une nouvelle surcharge do_casewise, alors nous nous assurerons de WideNetFilterdevenir une classe abstraite: nous devons traiter des transactions d'un nouveau type dans WideNetFilter(mais pas dans NameWatchlistFilter). C'est comme si le compilateur gardait une liste de choses à faire pour nous!



conclusions



J'ai anonymisé et extrêmement simplifié cet exemple par rapport au code avec lequel je devais travailler. Mais le contour général de la hiérarchie des classes était exactement cela, tout comme la logique fragile do_generic(txn) && do_casewise(txn)du code d'origine. Je pense que d'après ce qui précède, il n'est pas si facile de comprendre à quel point le bogue était imperceptiblement caché dans l'ancienne structure FeeTxn. Maintenant que je vous ai présenté cette version simplifiée (littéralement mâchée pour vous!), Je suis moi-même pratiquement surpris, la version originale du code était-elle si mauvaise? Peut-être pas ... après tout, ce code a fonctionné pendant un certain temps.

Mais j'espère que vous conviendrez que la version de refactoring utilisant CasewiseFilteret surtout WatchlistGroups'est avérée bien meilleure. Si je devais choisir laquelle de ces deux bases de code travailler, je n'hésiterais pas à prendre la seconde.


More articles:


All Articles