Русский
Русский
English
Статистика
Реклама

Перевод Наследование реализации в С. Реальная история

Привет, Хабр!

В поисках вдохновения, чем бы пополнить портфель издательства на тему С++, мы набрели на возникший словно из ниоткуда блог Артура О'Дуайера, кстати, уже написавшего одну книгу по C++. Сегодняшняя публикация посвящена теме чистого кода. Надеемся, что вам будут интересны как сам кейс, так и автор.


Чем больше я имею дело с классическим полиморфным кодом, тем сильнее ценю современные идиомы, которыми он оброс идиому невиртуального интерфейса, принцип подстановки Лисков, правило Скотта Майерса, согласно которому все классы должны быть либо абстрактными, либо финальными, правило, что любая иерархия должна быть строго двухуровневой, а также правило, согласно которому базовые классы выражают единство интерфейса, а не переиспользуют реализацию.

Сегодня я хочу показать фрагмент кода, демонстрирующий, как наследование реализации привело к проблемам, и какие именно паттерны я применил, чтобы его распутать. К сожалению, возмутивший меня код был настолько неразборчив, что здесь я решил показать его в упрощенном и немного предметно-ориентированном виде. Постараюсь поэтапно изложить всю проблему.

Этап 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 пытается помечать любую транзакцию, где имя ее исполнителя отличается от имени, указанного в счете. Ради примера предположим, что банк строго регламентирует: снимать деньги со счета может только владелец этого счета. Поэтому лишь class 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 polymorphic visit (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;        });    }};


Поэтому автора кода осенила (сарказм!) идея. Давайте реализуем в некоторых фильтрах чувствительность к регистру. Перепишем базовый класс 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. Но мы нарушаем один из принципов ООП, а именно, запрет на наследование реализации. Функция 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, применим принцип единственной ответственности. На данный момент NameWatchlistFilter решает две задачи: работает в качестве полноценного фильтра и владеет возможностью is_flagged. Давайте выделим is_flagged в самостоятельный класс WatchlistGroup, которому не требуется соответствовать API 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_flagged, не наследуя NameWatchlistFilter. В обоих фильтрах можно следовать известной рекомендации и предпочитать композицию наследованию, так как композиция это инструмент для переиспользования кода, а наследование нет.

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, получилась гораздо лучше. Если бы мне пришлось выбирать, с какой из двух этих баз кода работать, то я без колебаний взял бы вторую.
Источник: habr.com
К списку статей
Опубликовано: 24.10.2020 14:21:16
0

Сейчас читают

Комментариев (0)
Имя
Электронная почта

Блог компании блог компании издательский дом «питер»

Программирование

Совершенный код

C++

Проектирование и рефакторинг

С++

Чистый код

Ооп

Категории

Последние комментарии

  • Имя: Макс
    24.08.2022 | 11:28
    Я разраб в IT компании, работаю на арбитражную команду. Мы работаем с приламы и сайтами, при работе замечаются постоянные баны и лаги. Пацаны посоветовали сервис по анализу исходного кода,https://app Подробнее..
  • Имя: 9055410337
    20.08.2022 | 17:41
    поможем пишите в телеграм Подробнее..
  • Имя: sabbat
    17.08.2022 | 20:42
    Охренеть.. это просто шикарная статья, феноменально круто. Большое спасибо за разбор! Надеюсь как-нибудь с тобой связаться для обсуждений чего-либо) Подробнее..
  • Имя: Мария
    09.08.2022 | 14:44
    Добрый день. Если обладаете такой информацией, то подскажите, пожалуйста, где можно найти много-много материала по Yggdrasil и его уязвимостях для написания диплома? Благодарю. Подробнее..
© 2006-2024, personeltest.ru