[Программирование, Совершенный код, C++, Проектирование и рефакторинг] Наследование реализации в С++. Реальная история (перевод)
Автор
Сообщение
news_bot ®
Стаж: 6 лет 9 месяцев
Сообщений: 27286
Привет, Хабр!
В поисках вдохновения, чем бы пополнить портфель издательства на тему С++, мы набрели на возникший словно из ниоткуда блог Артура О'Дуайера, кстати, уже написавшего одну книгу по 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
===========
===========
Автор оригинала: Arthur O’Dwyer
===========Похожие новости:
- [Анализ и проектирование систем, Совершенный код, Проектирование и рефакторинг, API] Чего ждать при работе с API: 5 (не)обычных проблем при интеграции приложений
- [Разработка мобильных приложений, Flutter] Flutter: результаты опроса разработчиков за Q3 2020 (перевод)
- [Разработка веб-сайтов, Программирование, Dart, Flutter] Обновление роадмапа AngularDart (перевод)
- [PHP, Программирование] 2R2L кеширование
- [Программирование, Проектирование и рефакторинг, Разработка игр] Как мы пришли к реактивному связыванию в Unity3D
- [Анализ и проектирование систем, Программирование, Проектирование и рефакторинг, Управление персоналом, Управление разработкой] Человечная декомпозиция работы
- [Тестирование IT-систем, Программирование, Тестирование веб-сервисов] Как мы «разогнали» команду QA, и что из этого получилось
- [C++, Читальный зал, Конференции] Герб Саттер о будущем С++, работе в комитете, фонде C++ Foundation и синтаксическом сахаре
- [DIY или Сделай сам, Программирование микроконтроллеров] Дистанционное управление громкостью IP TV приставки при помощи Attiny13A
- [Высокая производительность, Python, Программирование, Машинное обучение] Deep Learning Inference Benchmark — измеряем скорость работы моделей глубокого обучения
Теги для поиска: #_programmirovanie (Программирование), #_sovershennyj_kod (Совершенный код), #_c++, #_proektirovanie_i_refaktoring (Проектирование и рефакторинг), #_s++ (С++), #_programmirovanie (программирование), #_chistyj_kod (чистый код), #_oop (ооп), [url=https://torrents-local.xyz/search.php?nm=%23_blog_kompanii_blog_kompanii_izdatelskij_dom_«piter»&to=0&allw=0&o=1&s=0&f%5B%5D=820&f%5B%5D=959&f%5B%5D=958&f%5B%5D=872&f%5B%5D=967&f%5B%5D=954&f%5B%5D=885&f%5B%5D=882&f%5B%5D=863&f%5B%5D=881&f%5B%5D=860&f%5B%5D=884&f%5B%5D=865&f%5B%5D=873&f%5B%5D=861&f%5B%5D=864&f%5B%5D=883&f%5B%5D=957&f%5B%5D=859&f%5B%5D=966&f%5B%5D=956&f%5B%5D=955]#_blog_kompanii_blog_kompanii_izdatelskij_dom_«piter» (
Блог компании Блог компании Издательский дом «Питер»
)[/url], #_programmirovanie (
Программирование
), #_sovershennyj_kod (
Совершенный код
), #_c++, #_proektirovanie_i_refaktoring (
Проектирование и рефакторинг
)
Вы не можете начинать темы
Вы не можете отвечать на сообщения
Вы не можете редактировать свои сообщения
Вы не можете удалять свои сообщения
Вы не можете голосовать в опросах
Вы не можете прикреплять файлы к сообщениям
Вы не можете скачивать файлы
Текущее время: 23-Ноя 00:42
Часовой пояс: UTC + 5
| Автор | Сообщение |
|---|---|
|
news_bot ®
Стаж: 6 лет 9 месяцев |
|
|
Привет, Хабр! В поисках вдохновения, чем бы пополнить портфель издательства на тему С++, мы набрели на возникший словно из ниоткуда блог Артура О'Дуайера, кстати, уже написавшего одну книгу по 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 =========== =========== Автор оригинала: Arthur O’Dwyer ===========Похожие новости:
Блог компании Блог компании Издательский дом «Питер» )[/url], #_programmirovanie ( Программирование ), #_sovershennyj_kod ( Совершенный код ), #_c++, #_proektirovanie_i_refaktoring ( Проектирование и рефакторинг ) |
|
Вы не можете начинать темы
Вы не можете отвечать на сообщения
Вы не можете редактировать свои сообщения
Вы не можете удалять свои сообщения
Вы не можете голосовать в опросах
Вы не можете прикреплять файлы к сообщениям
Вы не можете скачивать файлы
Вы не можете отвечать на сообщения
Вы не можете редактировать свои сообщения
Вы не можете удалять свои сообщения
Вы не можете голосовать в опросах
Вы не можете прикреплять файлы к сообщениям
Вы не можете скачивать файлы
Текущее время: 23-Ноя 00:42
Часовой пояс: UTC + 5