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