Когда-то в детстве я лежал на кровати и долго разглядывал узоры на старом советском ковре, видя в них животных и фантастические фигуры. Теперь я чаще смотрю на код, но в моем сознании по-прежнему рождаются похожие образы. Как и на ковре, эти образы складываются в повторяющиеся паттерны. Они могут быть как приятными, так и отталкивающими. Сегодня я хочу рассказать вам о таком неприятном паттерне, который встречается в программировании.
Сценарий
Представьте себе сервис, который обрабатывает запрос на регистрацию клиента и отправляет событие об этом в Kafka. В статье я покажу пример реализации, который считаю антипаттерном, и предложу исправленный вариант.
Вариант 1: Methodcentipede
В коде Java ниже представлен код класса RegistrationService, который обрабатывает запрос и отправляет событие.
public class RegistrationService {
private final ClientRepository clientRepository;
private final KafkaTemplate<Object, Object> kafkaTemplate;
private final ObjectMapper objectMapper;
public void registerClient(RegistrationRequest request) {
var client = clientRepository.save(Client.builder()
.email(request.email())
.firstName(request.firstName())
.lastName(request.lastName())
.build());
sendEvent(client);
}
@SneakyThrows
private void sendEvent(Client client) {
var event = RegistrationEvent.builder()
.clientId(client.getId())
.email(client.getEmail())
.firstName(client.getFirstName())
.lastName(client.getLastName())
.build();
Message message = MessageBuilder
.withPayload(objectMapper.writeValueAsString(event))
.setHeader(KafkaHeaders.TOPIC, "topic-registration")
.setHeader(KafkaHeaders.KEY, client.getEmail())
.build();
kafkaTemplate.send(message).get();
}
@Builder
public record RegistrationEvent(int clientId, String email, String firstName, String lastName) {}
}Структуру кода упрощенно можно представить в таком виде:
Здесь видно, что методы образуют неразрывную цепочку, по которой перетекает поток данных, как по длинной узкой кишке. Методы в середине этой цепочки ответственны не только за логику, непосредственно описанную в их теле, но и за логику вызываемых ими методов и их контракты (например, необходимость обработки определённых ошибок). Все методы, предшествующие вызываемому, наследуют всю его сложность. Например, если kafkaTemplate.send имеет сайд-эффект в виде отправки события, то и вызывающий его sendEvent приобретает тот же сайд-эффект. Метод sendEvent также несёт ответственность за сериализацию, включая обработку её ошибок. Тестирование отдельных частей кода усложняется тем, что нет возможности проверить каждую часть изолированно без использования моков.
Вариант 2: Исправленный вариант
Код:
public class RegistrationService {
private final ClientRepository clientRepository;
private final KafkaTemplate<Object, Object> kafkaTemplate;
private final ObjectMapper objectMapper;
@SneakyThrows
public void registerClient(RegistrationRequest request) {
var client = clientRepository.save(Client.builder()
.email(request.email())
.firstName(request.firstName())
.lastName(request.lastName())
.build());
Message<String> message = mapToEventMessage(client);
kafkaTemplate.send(message).get();
}
private Message<String> mapToEventMessage(Client client) throws JsonProcessingException {
var event = RegistrationEvent.builder()
.clientId(client.getId())
.email(client.getEmail())
.firstName(client.getFirstName())
.lastName(client.getLastName())
.build();
return MessageBuilder
.withPayload(objectMapper.writeValueAsString(event))
.setHeader(KafkaHeaders.TOPIC, "topic-registration")
.setHeader(KafkaHeaders.KEY, event.email)
.build();
}
@Builder
public record RegistrationEvent(int clientId, String email, String firstName, String lastName) {}
}
Схема представлена ниже:
Здесь видно, что метода sendEvent вовсе нет, и за отправку отвечает kafkaTemplate.send. Весь процесс построения сообщения для Kafka вынесен в отдельный метод mapToEventMessage. Метод mapToEventMessage не имеет сайд-эффектов, граница его ответственности четко очерчена. Исключения, связанные с сериализацией и отправкой сообщений, являются частью контракта отдельных методов и могут быть индивидуально обработаны.
Метод mapToEventMessage является чистой функцией. Когда функция детерминированная и не имеет побочных эффектов, мы называем её "чистой" функцией. Чистые функции:
проще читать,
проще отлаживать,
проще тестировать,
не зависят от порядка, в котором они вызываются,
просто запустить параллельно.
Рекомендации
Я бы предложил следующие техники, которые помогут избежать подобных антипаттернов в коде:
Подход Testing Trophy
Техника One Pile
Test-Driven Development (TDD)
Все эти техники тесно связаны и взаимно дополняют друг друга.
Testing Trophy
Это подход к покрытию кода тестами, при котором акцент делается на интеграционные тесты, проверяющие контракт сервиса в целом. Unit-тесты используются для отдельных функций, которые сложно или дорого тестировать через интеграционные тесты. Тесты с подобным подходом я описывал в своих статьях: Разносим по полочкам этапы тестирования http запросов в Spring, Повышение наглядности интеграционных тестов, Изоляция в тестах с Kafka.
One Pile
Эта техника описана в книге "Tidy First?" Кента Бека. Основная мысль: чтение и понимание кода сложнее, чем его написание. Если код разбит на слишком много мелких частей, может быть полезно сначала объединить его в одно целое, чтобы увидеть общую структуру и логику, а затем снова разделить на более понятные куски.
В контексте данной статьи предлагается не разделять код на методы до тех пор, пока он не будет обеспечивать выполнение требуемого контракта.
Testing Driven Development
Этот подход позволяет разделить усилия на написание кода для реализации контракта и на формирование дизайна кода. Мы не пытаемся сразу сделать хороший дизайн и написать код, соответствующий требованиям, а разделяем эти задачи. Процесс разработки выглядит следующим образом:
Пишем тесты для контракта сервиса, используя подход Testing Trophy.
Пишем код в стиле One Pile, добиваясь того, чтобы он обеспечивал выполнение требуемого контракта. Не обращаем внимания на качество дизайна кода.
Делаем рефакторинг кода. Весь код написан, у нас есть полное представление о реализации и возможных узких местах.
Заключение
В статье рассмотрен пример антипаттерна, который может привести к сложностям в поддержке и тестировании кода. Подходы, такие как Testing Trophy, One Pile и Test-Driven Development, позволяют структурировать работу таким образом, чтобы код не превращался в непроходимый лабиринт. Инвестируя время в правильную организацию кода, мы закладываем основу для долговременной устойчивости и простоты сопровождения наших программных продуктов.
Спасибо за внимание к статье, и удачи в вашем стремлении к написанию простого кода!
Комментарии (20)
DenSigma
23.08.2024 06:38Ну вот у вас-же наполовину уже сделано по-человечески. Детали реализации сохранения в базу вынесены в ClientRepository. Почему-же и с очередью сообщений вся логика и dependency зашита в
RegistrationService?Вынесите детали реализации отправки сообщений в отдельный репозиторий MessageRepsitory с определенным интерфейсом и кидайте сообщения оттуда. С помощью какого механизма -Кафка, кролика или еще чего, это не должно касаться сервиса. Надо будет - выкинете кафку, поставите рэббита.
Avvero Автор
23.08.2024 06:38А что значит по-человечески? Есть функциональные требования и не функциональные. И предложенная реализация отвечает им в полной мере. Архитектор и представитель бизнеса понимают риски и согласны на такое решение, оно их полностью устраивает. Дизайн должен быть прагматичным, а не быть витриной скилов разработчиков.
>Надо будет - выкинете кафку, поставите рэббита.Если нада будет, то придет задача и мы внесем правки в сервис. Нет ровно ни одной причины пытаться угадать, что там будет завтра и под это закладываться. Это ничем не лучше попытки гадания на таро.
DenSigma
23.08.2024 06:38По человечески, это вот так: https://ru.wikipedia.org/wiki/SOLID_(%D0%BF%D1%80%D0%BE%D0%B3%D1%80%D0%B0%D0%BC%D0%BC%D0%B8%D1%80%D0%BE%D0%B2%D0%B0%D0%BD%D0%B8%D0%B5)
Эти принципы были сформированы еще в начале двухтысячных годов, и у меня, честно говоря, вызывает изумление, что в 2024 году еще идет обсуждение каких-то паттернов-антипаттернов, не соответстующих этим принципам, изобретаются велосипеды и вообще, предлагается на суд общественности код, не соответствующий этим принципам.
SOLID, точнее, идеология Дяди Боба - это практически, промышленный стандарт организации кода.
Avvero Автор
23.08.2024 06:38+1Если уж мы начали говорить на языке эмпирических принципов, то невозможно пройти мимо старых добрых KISS и YAGNI. И вот тут хочется задать вам вопрос: а на чем основана ваша уверенность, что предложенное решение действительно необходимо? Это что, принцип ради принципа? Или, может, дизайн ради дизайна? Звучит как попытка украсить ёлку гирляндами там, где нужно просто подключить лампочку.
netch80
23.08.2024 06:38+2SOLID, точнее, идеология Дяди Боба - это практически, промышленный стандарт организации кода.
Удивительно, что в 2024 ещё находятся люди, которые этому верят, несмотря на всю практику и множество обоснованных (в отличие от) критических статей; несмотря на то, что Clean Code противоречит сама себе и предлагает ужасный код; несмотря на то, что есть множество более внятно формулированных и потому более реальных методик... которые ещё и работают.
SOLID же в основе неконкретен настолько, насколько это вообще может быть. 'S' просто непонятно в отрыве от любой реальности, и становится понятным только через конкретные требования вроде Information Expert из GRASP (более внятный набор принципов) и реалии конкретного требования к гибкости и тестировании - но "дядя Боб" об этом не скажет, напустив туману. 'O' вообще не имеет отношения к проектированию, оно про жизненный цикл с развитием. 'L' про стабильность интерфейсов (в самом широком смысле) и контрактов. Остальные два имеют смысл только в одной конкретной объектной модели и теряются при выходе за её пределы.
На самом деле смысл этого симулякра совсем другой: это выдача - для внешнего потребителя, которыми являются непрограммирующие менеджеры и прочее начальство - hard skills за soft skills. Используя стандартизованные термины из SOLID или паттернов GoF, программист может объяснять начальству проблемы на стандартизованном языке, который, благодаря шуму, оно уже знает и предполагает его услышать. Но это же загоняет программиста в прокрустово ложе разработки именно в этих понятиях - которое работает, в основном, в домене "внутреннего софта" по Спольски, где и сконцентрировано основное начальственное невежество. Так что в целом оно скорее позитивно. Но - именно для программистов я бы желал, чтобы они понимали, насколько всё это чистейший симулякр.
aaoo
23.08.2024 06:38за сколько лет работы ни разу не приходилось объясняться терминами SOLID с непрогоаммистами. зачем вы вообще кому-то не коллеге рассказываете что-то подобное? по солид, а можете показать проект, который не использует эти принципы, поддерживается и развивается достаточно долго вменяемым количеством разработчиков, при этом каждое изменение это не переписать 70% кода. это не троллинга ради, мне действительно интересна альтернатива.
Avvero Автор
23.08.2024 06:38На сколько я понял суть претензии к SOLID в контексте обсуждения заключается в том, что им пытаются обосновать любое нагромождение бессмысленных абстракций
aaoo
23.08.2024 06:38а, ну да. во всём важна умеренность и рациональное зерно. 100% - абстракций ради абстракций не нужно. но вот по моему опыту, "лёгкость" работы с проектом который пытается в солид и который не пытается где-то 8 к 2 соответственно
netch80
23.08.2024 06:38за сколько лет работы ни разу не приходилось объясняться терминами SOLID с непрогоаммистами. зачем вы вообще кому-то не коллеге рассказываете что-то подобное?
Начальство разное бывает.
а можете показать проект, который не использует эти принципы, поддерживается и развивается достаточно долго вменяемым количеством разработчиков, при этом каждое изменение это не переписать 70% кода.
Да половина FOSS мира такая. GCC. Linux (как минимум ядро). FreeBSD, NetBSD, OpenBSD (полный комплект). Причём я не говорю о специфически ассемблерных компонентах или тому подобном - а исключительно о центральной логике. Или вот загляните в .NET runtime. Код gc это полтора мегабайта плетёной логики только в одном центральном файле. И работает же, неплохо работает. У меня есть примеры и пожёстче, но я отложу их для подходящего случая.
На самом деле и в них полно случаев принципов из SOLID. Single responsibility - контролирует, чтобы не было посторонней функциональности там, где она не нужна и мешает гибкости. Liskov substitution principle - например, работа с файлами в ядре через вызов функций в softc-структурах - полный аналог полиморфизма на виртуальных функциях с выполнением контракта для каждого вызова (грубо говоря, read() не выполняет запись). Аналогично для VFS и массы других уровней. Open/close выражается в требованиях стабильности API ядра и системных библиотек. И так далее. Но главное то, что они не циклятся на этих принципах! Где они нужны - они соблюдаются. Где нет - их игнорируют. Если они мешают эффективности - принципы идут нафиг. Базовые требования там совсем другие.
mvv-rus
23.08.2024 06:38Во-первых, мне вообще не нравится слово "антипатерн". Потому что все эти приемы проектирования имеют и достоинства, и недостатки, а потому - свою область применения. А ярлык "антипатерн" отвергает прием проектирования вообще и всегда, в духе высказывания "четыре хорошо - две плохо".
Конкретно по вашему примеру: если формат сообщения специфичен для транспорта, то вытаскивать этот специфичный формат наружу плохо: через Message наружу вылазит sequential coupling, и при попытке модификации кода оно себя проявит: например, если mapToEventMessage тоже будет реализован через аггрегацию, то в дальнейшем при замене kafkaTemplate на другой транспорт влегкую можно забыть поменять реализацию создания сообщений. Наследование (ещё один якобы "антипаттерн") может несколько сгладить проблему - поскольку производный класс выступит абстрактной фабрикой, создающей согласованные друг с другом реализации аггрегируемых компонентов, но оно указанную coupling не устраняет.
С другой стороны, реализация через sendEvent не столь страшна. Наприме, она тоже вполне может абстрагиоваться от обработки ошибок на нижележащих уровнях - особенно если все эти ошибки будут обернуты в исключения одного и того же типа : sendEvent получит сведения, что попытка послать сообщение провалилась (а registerClient все равно необходимо это знать и где-то обрабатывать, так почему бы не в sendEvent), выполнит необходимую очистку (если она необходима) и передаст исключение наверх, не вдаваясь в его детали.
Если там, наверху, эти детали кому-то нужны (например, статистику работоспособности транспорта сообщений собрать), то их можно передать, к примеру, через вложенное исключение.
А вот если формат сообщения универсален для разных транспортов, то тут эта самая "сороконожка" действительно не нужна и даже вредна.
Avvero Автор
23.08.2024 06:38В статье описан антипаттерн неверной декомпозиции кода и предложен вариант, лишенный изначальных недостатков. Если вы считаете, что аргументы неверные или вывод логически необоснован, то не могли бы вы быть более конкретным и указать на те аргументы, которые неверны, или на ошибку в логике суждений.
Касательно ваших воображаемых ситуаций.
если формат сообщения специфичен для транспорта, то вытаскивать этот специфичный формат наружу плохо
Формат сообщения в примере не специфичен для транспорта.
поскольку производный класс выступит абстрактной фабрикой
Попытка решить проблему дизайна через сокрытие за абстракциями ничего, кроме избыточной сложности, не приносит. Существует несколько известных мне прагматичных принципов, указывающих на необходимость введения абстракций — правило триангуляции и правило трёх конвертов. Оба утверждают, что в данном коде абстракции не нужны.
может несколько сгладить проблему
Подобным образом слой свежей шпатлёвки сглаживает неровности кузова машины.
при замене kafkaTemplate на другой транспорт влегкую можно забыть поменять реализацию создания сообщений
Утверждение указывает на тот факт, что в сервисе не предполагается наличие каких-либо тестов. Обратите, пожалуйста, внимание на раздел "Рекомендации" и роль тестов в решении проблем дизайна. Возможно, уделение тестам достаточного внимания укоренит мысль о том, что нельзя при разработке забыть поменять реализацию создания сообщений.
она тоже вполне может абстрагироваться от обработки ошибок на нижележащих уровнях
Абстрагирование действительно может скрыть не только детали обработки ошибок, но и ошибки дизайна. Чем больше абстракций, тем глубже кроличья нора. Использование подхода One pile позволит в эту нору не угодить.
В целом я хотел бы подчеркнуть, что дизайн приложения должен быть прагматичным, избегать излишнего кода и ненужных архитектурных решений. Если для сервиса из примера появятся новые требования по функциональности, именно тогда следует исходя из этих конкретных требований выстраивать новый дизайн, если старый больше не подходит. Выводить не нужные в данный момент абстракции на основе лишь предположений о том, что может случиться с кодом в будущем, является всего лишь спекуляцией. Это буквально ничем не отличается от гадания.mvv-rus
23.08.2024 06:38В статье описан антипаттерн неверной декомпозиции кода и предложен вариант, лишенный изначальных недостатков. Если вы считаете, что аргументы неверные или вывод логически необоснован, то не могли бы вы быть более конкретным и указать на те аргументы, которые неверны, или на ошибку в логике суждений.
Дык, я же вам на это и указывал. Но если это требует повторения - повторю: верная или неверная декомпозиция кода - это зависит от решаемой задачи, от контекста. И я показал вам, что в определенном контексте, который вполне подходит к предмету статьи, ваш подход хуже, а этот, якобы "антипатерн" - наоборот, лучше. То есть, ваш вывод имеет гораздо более узкую область применения, чем можно подумать, исходя из текста статьи. Но в этой области он верен, не спорю. Но статья эту область применимости не оговаривает, поэтому ваш вывод в этом, более широком, контексте неверен.
Формат сообщения в примере не специфичен для транспорта.
Если бы на это было указано в статье, моих возражений бы не было. Но было ли это указано?
Попытка решить проблему дизайна через сокрытие за абстракциями ничего, кроме избыточной сложности, не приносит.
Избыточна ли эта сложность или нет - это зависит от конкретной задачи. Если выбрать неверный уровень сложности, то можем получить, с одной сторны, "FizzBuzz Enterprise edition", с другой - матерое, очень упорно и нередко успешно сопротивляющееся любой модификации, legacy с прибитыми гвоздями друг к другу частями.
Выбор нужного уровня абстракции - это вопрос искусства автора программы, причем - именно искусства: хорошего работающего всегда способа ("серебяной пули") нет. Ну, а вообще без абстракций программирование в современном виде просто невозожно, поскольку переменная - это уже абстракция.
Впрочем, к термину "абстрактная фабрика" все эти разговоры про абстракции имеют слабое отношение: это - устоявшееся за три десятка лет, прошедших с выхода известной книги от Четверки(GoF) название конкретного приема программирования.
Утверждение указывает на тот факт, что в сервисе не предполагается наличие каких-либо тестов. Обратите, пожалуйста, внимание на раздел "Рекомендации" и роль тестов в решении проблем дизайна. Возможно, уделение тестам достаточного внимания укоренит мысль о том, что нельзя при разработке забыть поменять реализацию создания сообщений.
Ваше возражение похоже на возражения поклонников языков типа JavaScript или Python против статической типизации: мол, зачем она нужна, если все можно проверить тестами. Такие возражения игнорируют то, что ошибку лучше не допускать, а допустив - обнаружить как можно быстрее: при компиляции или даже раньше. Тесты - уже следующая линия обороны против ошибок, выявление и исправление ошибок тестами обходится дороже (но дешевле, конечно, чем исправлять ошибки на бою).
Выводить не нужные в данный момент абстракции на основе лишь предположений о том, что может случиться с кодом в будущем, является всего лишь спекуляцией.
Это - с одной стороны. А с другой, выбранную заранее архитектуру сложно поменять в процессе модификации приложения - а модификация сколь-нибудь успешно работающего приложения по жизни потребуется обязателно. Вот и приходится балансировать, выбирая без достаточной информации. И чтобы это делать успешно, нужен пресловутый опыт. А ещё - чутьё, которое обычно на основании опыта и появляется.
В целом я хотел бы подчеркнуть, что дизайн приложения должен быть прагматичным, избегать излишнего кода и ненужных архитектурных решений.
Дык, кто бы спорил? Только вот проблема - в том, чтобы понять какой код - излишний, а архитектурные решения - ненужные. И решать ее можно только творчески, для каждого конкретного случая отдельно. А для выбора решения - привыкнуть пользоваться всем богатсвом приемов, а не отвергать их бездумно, потому что они, якобы, "антипаттерн".
Собственно, в этом и основная мысль моего комментария: не надо бездумно записывать в "антипаттерны" никакие приемы программирования. Ибо "и терпентин на что-нибудь полезен!"(с)
Avvero Автор
23.08.2024 06:38повторю: верная или неверная декомпозиция кода - это зависит от решаемой задачи, от контекста.
Это не так, я вполне четко указал, почему декомпозиция была не верной и чем лучше чистая функция. Чистая функция дешевле в поддержке чем нечистая, это факт.
И я показал вам, что в определенном контексте, который вполне подходит к предмету статьи, ваш подход хуже
Тут вы глубоко заблуждаетесь, пространные рассуждения о несуществующих проблемах не являются аргументацией. При этом вы не привели контр аргументов к моим доводам.
Если бы на это было указано в статье, моих возражений бы не было. Но было ли это указано?
В статье указан, что речь про Kafka. Если вы задаете вопросы о формате представления данных, то можете обратиться к ее документации и там найти ответ на свой вопрос. В контексте же статьи, эти детали не важны.
Избыточна ли эта сложность или нет - это зависит от конкретной задачи.
В контексте примера статьи предложенные вами решения это избыточная сложность, обоснованная спекулятивным представлением о будущем.
Выбор нужного уровня абстракции - это вопрос искусства автора программы
Основная цель архитектуры приложения это снижение издержек на разработку и сопровождение. Все разговоры об искустве в контексте темы - это продукт богатого воображения и не прагматичного подхода к разработке.
хорошего работающего всегда способа ("серебяной пули") нет.
Есть, принцип бритвы Оккама - "Не следует множить сущее без необходимости", влияние этой идеи прослеживается в огромном количестве эмпирических принципов.
Такие возражения игнорируют то, что ошибку лучше не допускать
Вы на полном серьезе утверждаете, что это помогает не допускать ошибок лишь правильное желание их не допускать. Вы поклонник аффирмаций?
Тесты - уже следующая линия обороны против ошибок, выявление и исправление ошибок тестами обходится дороже
Это не так. Тесты это дешевый способ проверки того, что программа выполняет требуемый контракт.
Только вот проблема - в том, чтобы понять какой код - излишний, а архитектурные решения - ненужные.
Такой проблемы не существует, код который не несет пользы - лишний. Например предложенные вами абстракции.
И решать ее можно только творчески, для каждого конкретного случая отдельно.
Это только при недостатке знания, когда "Голь хитра, голь мудрена, голь на выдумки горазда". Существует целый ворох принципов, которые говорят - не пишите лишний код, пишите простой код, не нагораживайте заранее ничего.
В целом было бы круто, если бы вы подтверждали свою позицию хоть какими-то аргументами. К примеру утверждение "выявление и исправление ошибок тестами обходится дороже" требует обоснований.
sshmakov
А еще registerClient упадет, если упали на маппинге или не удалось достучаться до Кафки, хотя сама регистрация клиента была успешно выполнена. Впрочем, это тема уже для другой статьи.
Avvero Автор
DenSigma
И что здесь неправильно?
sshmakov
Мне кажется, потребителю сервиса надо знать, что регистрация клиента прошла или нет, а не то, отправил ли сервис что-то куда-то. Потому что если сервис регистрации упал с ошибкой, то потребитель попробует повторить регистрацию, но в этот раз у него не получится, так как клиент уже зарегистрирован, просто потребитель не в курсе.
То есть сервис должен падать только при невозможности выполнения его основной функции - регистрации, а все сопутствующие действия либо проводить без прокидывания exception наверх, либо, если это важно, откатывая регистрацию. Тогда у потребителя будет четкое понимание: выполнилось успешно - значит регистрация прошла; exception - значит регистрации не случилось.
Avvero Автор
Возможно вы знаете больше о требованиях к это сервису, чем ее заказчик, прощу связаться со мной и обсудить позицию продукт овнера.
sshmakov
Как не каждый разработчик стремится стать инженером, так не каждый аналитик стремится стать продукт овнером.