24.10.2020 11:15
ph_piter 11 4200 Источник
Привет, Хабр!

В поисках вдохновения, чем бы пополнить портфель издательства на тему С++, мы набрели на возникший словно из ниоткуда блог Артура О'Дуайера, кстати, уже написавшего одну книгу по 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, получилась гораздо лучше. Если бы мне пришлось выбирать, с какой из двух этих баз кода работать, то я без колебаний взял бы вторую.