В прошлой части мы рассказали, что такое чистый код и какие принципы нужно соблюдать, чтобы его написать. В новой — поговорим про плохой код: перечислим проблемы и покажем, как их решать.
Эта статья — «прожарка». Она не отвечает на все вопросы и не содержит иллюстрации ко всей критике. В будущем мы шаг за шагом более атомарно будем всё разгребать.
Максим Морев Software Craftsmanship энтузиаст, CTO
Ваганов Вадим Software Engineer, Head of Profession backend-разработки
Всё, рассказанное ниже, база, которую мы взяли из своего опыта, и теперь хотим поделиться.
Представьте, вы — разработчик. Только пришли в команду, познакомились с классными коллегами. И увидели… сервис. Такой, как этот. Читать код невозможно, тестов почти нет, каждый класс «кусается» (ещё и на ChatGPT не свалишь).
Говорить про тесты здесь не будем — это большая тема, которой лучше посвятить отдельный текст.
Если вы читали Роберта Мартина или, как минимум, мою предыдущую статью — появится мысль: «Нужно срочно покинуть эту команду». Но лучше выдохнуть и проанализировать код — его можно превратить в гармоничное приложение, которое легко прочитать, понять и, если нужно, доработать.
Сканируем методы, классы и структуру пакетов.
Мы постарались сделать «прожарку» максимально понятной. Но всё равно может быть тяжеловато.
Довольно простой REST-клиент на базе WebClient. По сигнатуре функция sendRequest соответствует нашему гайду. Но 82 строки для подобного класса — это много. Так что стоит упростить: разбить код на блоки и вытащить каждый в отдельную функцию (метод класса в ООП).
Также RESTClient важно покрыть тестами.
MqConfiguration — контейнер конфигурации, который содержит класс MqServiceCfg. Захардкоженные значения полей timeout, messagePipeline и type смущают. Лучше вынести значения в конфиг-файл.
Класс WebClientConfiguration стоит проверить тестами.
ProcessException — класс, который нигде не используется. Просто удалить.
Опустим классы-контейнеры без инкапсулированной логики работы с данными. И остановлюсь на проблемах в других классах.
Смарт-конструктор emerge может выкинуть исключение на этапе десериализации из файла.
Нужно это предусмотреть и обработать. Например. обернуть Result.runCatching{ }. Тогда emerge будет возвращать Result, то есть однозначно говорить о том, что функция (метод) может зафэйлиться при исполнении и вернуть Two Track Type — или результат выполнения в поле success или failure.
Если вы отдельно обрабатываете исключения, можно обернуть их в удобный тип.
Валидацию формата полей класса dateFrom, dateTo лучше вынести в функцию emerge и если на вход влетели битые данные — вернуть понятную ошибку.
Класс — DTO, принадлежит слою представления, перегружен логикой, которой в DTO вообще быть не должно по определению. Её нужно вынести в соответствующие классы уровня бизнес-логики.
Поле класса body (31 строка) лучше сделать пустой строкой по-умолчанию — так приближаемся к Null Safety universe.
Поля просто перекладываются из входных параметров функции в поля класса. Умный конструктор говорит «тут возможна ошибка создания валидного объекта, поэтому я верну Result», который нам не нужен. Поэтому лучше заменить его на простой of метод.
В первую очередь — что за заклинание вместо названия? Переименовать.
Нэйминг — это важно. Но мы на данном этапе не знаем, какое имя выбрать, нужно будет погрузиться в аналитику.
Во-вторых, в 71 строке мы генерируем json-строку из класса, который собирается из тела body-запроса. И если GetWalletBalanceRequest зафэйлится и не сможет собраться из строки запроса, которая должна содержать JSON, мы получим необработанный Exception. Это плохо, так в коде преднамеренно заложена ошибка и не заложена обработка этой ошибки.
Лучше собрать класс в одном месте с HttpRequestEnvelope и GetWalletBalanceRequest.
В-третьих, в 63 строке вместо fold стоит использовать Result.map — он для этого и создан. Или вовсе — передавать на вход в данный метод не Result, а HttpRequestEnvelope.
Проектируйте систему так, чтобы она работала с валидными объектами — это возможно и помогает смоделировать процесс более надёжно. Так, на слое, где можно вызвать «Пут Пс Реквест Ин Боди» — что бы это ни значило — стоит обработать возможную ошибку, а не прокидывать её в наш замечательный метод.
Например так:
Кроме того, validatedRequest содержит в себе validatePsRequest.
Но валидации для PsGetBalanceRequest тут не место. Она должна находиться в умном конструкторе PsGetBalanceRequest. Да и саму функцию validatePsRequest (в 24 строки) можно описать элегантней.
Вопрос к nullable-полям. Почему в запросе перекладчика могут отсутствовать:
Нужно чётко описать контракт в README и вместе с аналитиком прояснить, в каких случаях эти параметры могут быть null. Часто можно прийти к выводу, что поля класса сделаны nullable «на всякий случай» потому что лень писать качественно.
Функция не используется — удалить.
Это просто мэпер, поэтому смарт-конструктор тут не нужен. Например можно использовать of для такой задачи.
И выглядеть это будет так:
В этой функции:
А ещё стоит читать документацию к классу Result на сайте Kotlin и использовать методы по назначению и на нужном уровне. Но с этим мы разберемся подробнее при рефакторинге.
Дружелюбный Alt + F7 показал, что HttpResponseEnvelope, MqResponseMessage, RestServiceRequest и PsGetBalanceRequest не используются — удаляем.
Мы работаем не с библиотекой, так что IDE подсказывает, что нужно убрать, через warning. Можно еще подключить плагины: SonarLint, SonarAnalyzer и так далее — они хорошо помогают.
Кстати, есть хорошая рекомендация от Марка Симана (кинга «Код, который умещается в голове»): делайте предупреждения ошибками и не допускайте пул реквесты с warning в коде.
Метод registerJmsListener создает слушатель на очередь, когда мы поднимаем конфигурацию в классе MqServiceConfiguration. Описать слушатель на конкретной задаче лучше явно. Это упростит восприятие и сократит конфигурационную обертку.
При рефакторинге мы явно создадим слушатель в так называемом Buble Context — и покажем, как просто протестировать этот код.
Содержит два небезопасных метода, которые могут выкинуть исключения:
Зачем нам вообще и асинхронный, и синхронный методы, когда можно завернуть IO-операцию в асинхронный стек реактивщины и оставить только publishToMq, переименовав метод в publish.
Содержит бизнес-логику обогащения запроса заголовками. Такое сложно тестировать. Так что функцию следует вытащить в доменный объект и тестировать явно юнит-тестом.
Функция receiveMessage описывает громоздкий контракт, которому должны соответствовать сервисы обработки разных бизнес-потоков для разных слушателей. Если нужно передать много параметров — лучше оберните параметры в класс, получится проще и «чище».
Кроме того, большинство параметров передать инъекцией. Попробуйте написать Rest Controller, который описывает API URL-ресурса, и в сигнатуру метода пердеайте конфигурацию rest-сервиса, rest client, который понадобится под капотом на уровне application service, и mq publisher на всякий случай.
Application — сервис, помеченный аннотацией фреймворка как компонент. Если имплементировать интерфейс, начинаются очень странные дела в теле функции (честно, мы ее не придумали).
Входные параметры присваиваются полям класса, потому что разработчик так передал компоненты, конфигурацию и REST-клиент. Он логирует заголовки и тело запроса и пробрасывает входной параметр message в метод process.
А ещё код как бы поощряет невалидные ситуации. Он написан в ущерб тестам и может сломать контракт.
Пройдёмся по ошибкам в классе:
Во-первых, нужно уменьшить количество входных параметров.
Во-вторых, в строке 106 — вместо fold можно применить map.
В третьих, прочитаем тело функции:
То есть функция должна отправить сообщение с квитанцией в очередь mq. Но из имени не понятно, что делается в теле. И происходящее описано очень сложно. Решение — переименовать функцию, отправить квитанцию на вход, подать один параметр, в теле отправить квитанцию в mq.
Эта функция буквально говорит то же, что и предыдущая: «Я содержу много бизнес-логики преобразований. Меня неудобно читать. Во мне много входных параметров. Во мне неверно используют Result».
Кроме того, не стоит кидать fold в fold через map. А map{it} мэпит в самого себя строка 151.
createMessageForReceipt после упрощения будет возвращать объект, который легко протестировать.
Во-первых, mqConfig передавать не нужно, это — параметр класса, его класс получает в виде инъекции. Во-вторых, бизнес-логику конвертации стоит выносить из сервиса в класс, так как в данном приватном методе её не протестировать наглядно и просто.
Те же замечания что и к предыдущей функции.
Те же замечания что и к предыдущей функции.
В HttpResponseEnvelope.emerge нет умного конструктора, поэтому функцию назвать нужно было map и использовать просто в of.
Кроме того, нужно правильно применить Result в классе HttpResponseEnvelope. Сейчас, этот метод (функция) возвращает Result просто потому что.
Судя по развилке, функция делает несколько дел сразу.
К тому же функция submitMqReceipt принимает на вход слишком много лишних параметров.
Функция submitMqResponse чуть проще, но ошибки те же: выбор, конвертация, отправка.
После форматирования чуть проще воспринять.
В следующей части начнём рефакторить код и разбирать всё подробнее.
Эта статья — «прожарка». Она не отвечает на все вопросы и не содержит иллюстрации ко всей критике. В будущем мы шаг за шагом более атомарно будем всё разгребать.
Сперва «высечем» тезисы: как нельзя писать
Всё, рассказанное ниже, база, которую мы взяли из своего опыта, и теперь хотим поделиться.
- НЕ пишите, если не нравится — круто, если код вас увлекает, и время в компании IDE летит. Но если нет — выберите что-то кроме программирования. В IT много интересной работы.
- НЕ пишите слепо по ТЗ аналитика — лучше прописывать требования совместно (или хотя бы разобраться, что конкретно хочет аналитик). Потому что код — ответственность разработчика, и разгребать проблемы придется именно ему.
- НЕ пишите, пока кратко и четко не описали проблему — её нужно зафиксировать в README репозитория на понятном пользователю языке. В идеале — описать поведение системы так, чтобы его можно было легко переложить в тесты. Это поможет смотреть на систему с перспективы её поведения.
- НЕ пишите без предварительного проектирования — можно заранее визуализировать архитектуру приложения. Спросите себя, из каких компонентов будет состоять приложение, почему именно из них, какие роли будут исполнять классы на каждом уровне.
А теперь — сам «плохой код»
Представьте, вы — разработчик. Только пришли в команду, познакомились с классными коллегами. И увидели… сервис. Такой, как этот. Читать код невозможно, тестов почти нет, каждый класс «кусается» (ещё и на ChatGPT не свалишь).
Говорить про тесты здесь не будем — это большая тема, которой лучше посвятить отдельный текст.
Если вы читали Роберта Мартина или, как минимум, мою предыдущую статью — появится мысль: «Нужно срочно покинуть эту команду». Но лучше выдохнуть и проанализировать код — его можно превратить в гармоничное приложение, которое легко прочитать, понять и, если нужно, доработать.
Сканируем методы, классы и структуру пакетов.
Мы постарались сделать «прожарку» максимально понятной. Но всё равно может быть тяжеловато.
Пакет client.RESTClient
Довольно простой REST-клиент на базе WebClient. По сигнатуре функция sendRequest соответствует нашему гайду. Но 82 строки для подобного класса — это много. Так что стоит упростить: разбить код на блоки и вытащить каждый в отдельную функцию (метод класса в ООП).
Также RESTClient важно покрыть тестами.
Пакет configuration
MqConfiguration — контейнер конфигурации, который содержит класс MqServiceCfg. Захардкоженные значения полей timeout, messagePipeline и type смущают. Лучше вынести значения в конфиг-файл.
Класс WebClientConfiguration стоит проверить тестами.
Пакет exceptions
ProcessException — класс, который нигде не используется. Просто удалить.
Пакет mq
Опустим классы-контейнеры без инкапсулированной логики работы с данными. И остановлюсь на проблемах в других классах.
GetWalletBalanceRequest
Смарт-конструктор emerge может выкинуть исключение на этапе десериализации из файла.
Нужно это предусмотреть и обработать. Например. обернуть Result.runCatching{ }. Тогда emerge будет возвращать Result, то есть однозначно говорить о том, что функция (метод) может зафэйлиться при исполнении и вернуть Two Track Type — или результат выполнения в поле success или failure.
Если вы отдельно обрабатываете исключения, можно обернуть их в удобный тип.
Валидацию формата полей класса dateFrom, dateTo лучше вынести в функцию emerge и если на вход влетели битые данные — вернуть понятную ошибку.
HttpRequestEnvelope
Класс — DTO, принадлежит слою представления, перегружен логикой, которой в DTO вообще быть не должно по определению. Её нужно вынести в соответствующие классы уровня бизнес-логики.
Поле класса body (31 строка) лучше сделать пустой строкой по-умолчанию — так приближаемся к Null Safety universe.
Функция emerge
Поля просто перекладываются из входных параметров функции в поля класса. Умный конструктор говорит «тут возможна ошибка создания валидного объекта, поэтому я верну Result», который нам не нужен. Поэтому лучше заменить его на простой of метод.
Функция putPsRequestInBody
В первую очередь — что за заклинание вместо названия? Переименовать.
Нэйминг — это важно. Но мы на данном этапе не знаем, какое имя выбрать, нужно будет погрузиться в аналитику.
Во-вторых, в 71 строке мы генерируем json-строку из класса, который собирается из тела body-запроса. И если GetWalletBalanceRequest зафэйлится и не сможет собраться из строки запроса, которая должна содержать JSON, мы получим необработанный Exception. Это плохо, так в коде преднамеренно заложена ошибка и не заложена обработка этой ошибки.
Лучше собрать класс в одном месте с HttpRequestEnvelope и GetWalletBalanceRequest.
В-третьих, в 63 строке вместо fold стоит использовать Result.map — он для этого и создан. Или вовсе — передавать на вход в данный метод не Result, а HttpRequestEnvelope.
Проектируйте систему так, чтобы она работала с валидными объектами — это возможно и помогает смоделировать процесс более надёжно. Так, на слое, где можно вызвать «Пут Пс Реквест Ин Боди» — что бы это ни значило — стоит обработать возможную ошибку, а не прокидывать её в наш замечательный метод.
Например так:
Кроме того, validatedRequest содержит в себе validatePsRequest.
Но валидации для PsGetBalanceRequest тут не место. Она должна находиться в умном конструкторе PsGetBalanceRequest. Да и саму функцию validatePsRequest (в 24 строки) можно описать элегантней.
HttpResponseEnvelope
Вопрос к nullable-полям. Почему в запросе перекладчика могут отсутствовать:
- method;
- service;
- operation;
- body;
- headers.
Нужно чётко описать контракт в README и вместе с аналитиком прояснить, в каких случаях эти параметры могут быть null. Часто можно прийти к выводу, что поля класса сделаны nullable «на всякий случай» потому что лень писать качественно.
toJsonString
Функция не используется — удалить.
47 строка, emerge
Это просто мэпер, поэтому смарт-конструктор тут не нужен. Например можно использовать of для такой задачи.
И выглядеть это будет так:
62 строка, emerge
В этой функции:
- не нужно создавать клон функции на 47-ой строке и передавать на вход Result — стоит удалить метод.
- нужен map вместо fold.
А ещё стоит читать документацию к классу Result на сайте Kotlin и использовать методы по назначению и на нужном уровне. Но с этим мы разберемся подробнее при рефакторинге.
86 строка emerge(correlationId: String, ex: Throwable):
Дружелюбный Alt + F7 показал, что HttpResponseEnvelope, MqResponseMessage, RestServiceRequest и PsGetBalanceRequest не используются — удаляем.
Мы работаем не с библиотекой, так что IDE подсказывает, что нужно убрать, через warning. Можно еще подключить плагины: SonarLint, SonarAnalyzer и так далее — они хорошо помогают.
Кстати, есть хорошая рекомендация от Марка Симана (кинга «Код, который умещается в голове»): делайте предупреждения ошибками и не допускайте пул реквесты с warning в коде.
Пакет service
MqHttpService
Метод registerJmsListener создает слушатель на очередь, когда мы поднимаем конфигурацию в классе MqServiceConfiguration. Описать слушатель на конкретной задаче лучше явно. Это упростит восприятие и сократит конфигурационную обертку.
При рефакторинге мы явно создадим слушатель в так называемом Buble Context — и покажем, как просто протестировать этот код.
MqPublisher
Содержит два небезопасных метода, которые могут выкинуть исключения:
- Первый — асинхронный метод, который оборачивает отправку сообщения в очередь на строке 22 в mono. Вызов может выкинуть исключение, как минимум, в двух очевидных случаях: если очереди не существует или отвалилось подключение к MQ.
- Второй — небезопасный синхронный метод отправки сообщения в очередь.
Зачем нам вообще и асинхронный, и синхронный методы, когда можно завернуть IO-операцию в асинхронный стек реактивщины и оставить только publishToMq, переименовав метод в publish.
Функция fillMessageWithHeaders
Содержит бизнес-логику обогащения запроса заголовками. Такое сложно тестировать. Так что функцию следует вытащить в доменный объект и тестировать явно юнит-тестом.
Интерфейс PipelineService
Функция receiveMessage описывает громоздкий контракт, которому должны соответствовать сервисы обработки разных бизнес-потоков для разных слушателей. Если нужно передать много параметров — лучше оберните параметры в класс, получится проще и «чище».
Кроме того, большинство параметров передать инъекцией. Попробуйте написать Rest Controller, который описывает API URL-ресурса, и в сигнатуру метода пердеайте конфигурацию rest-сервиса, rest client, который понадобится под капотом на уровне application service, и mq publisher на всякий случай.
PipelineServicePS-сервис приложения
Application — сервис, помеченный аннотацией фреймворка как компонент. Если имплементировать интерфейс, начинаются очень странные дела в теле функции (честно, мы ее не придумали).
Входные параметры присваиваются полям класса, потому что разработчик так передал компоненты, конфигурацию и REST-клиент. Он логирует заголовки и тело запроса и пробрасывает входной параметр message в метод process.
А ещё код как бы поощряет невалидные ситуации. Он написан в ущерб тестам и может сломать контракт.
Пройдёмся по ошибкам в классе:
- processMessage. Метод на 30 строк, который состоит из секции настройки переменных (больше похожей на ловушки). Разберёмся с аналитиком по контракту и упростим.
- correlationId. 64-ая строка Устанавливается Elvis operator рандомно и при этом отсутствует в заголовке. На деле correlationId — должен передаваться в заголовке всегда, иначе система выдаст ошибку и сообщит об этом. Подробнее можно почитать тут.
- url. Глушим отсутствие в заголовке значения метода. Так делать нельзя. Нужно проверить контракт, и если url не задан — вернуть сообщение об ошибке.
- Вместо того чтобы писать комментарий ниже, лучше сразу найти способ или создать тикет и добавить его в комментарий — задача будет реализована в рамках 20-30% техдолга.
- httpReq должна быть неизменной — val.
Функция acceptedMqReceipt
Во-первых, нужно уменьшить количество входных параметров.
Во-вторых, в строке 106 — вместо fold можно применить map.
В третьих, прочитаем тело функции:
- Строка 107. Если на вход пришел успешный запрос, вызываем функцию createMessageForReceipt, входных параметров в которую слишком много.
- Строка 109. Вызываем map в котором лежит бизнес-логика преобразования сообщения, созданного в функции createMessageForReceipt.
- Строка 112. Отправляем ответ на что-то неизвестное, судя по имени jmsResponse.
- Строка 100. Если приходит успешный HttpRequestEnvelope, который был собран из строки 75 и провалидирован, берем некое Ps, помещаем в HttpRequestEnvelope и проваливаемся в странную функцию acceptedMqReceipt, где создаем сообщение для квитанции, превращаем его в jmsResponse и отправляем в строку 112
То есть функция должна отправить сообщение с квитанцией в очередь mq. Но из имени не понятно, что делается в теле. И происходящее описано очень сложно. Решение — переименовать функцию, отправить квитанцию на вход, подать один параметр, в теле отправить квитанцию в mq.
Функция fun createMessageForReceipt
Эта функция буквально говорит то же, что и предыдущая: «Я содержу много бизнес-логики преобразований. Меня неудобно читать. Во мне много входных параметров. Во мне неверно используют Result».
Кроме того, не стоит кидать fold в fold через map. А map{it} мэпит в самого себя строка 151.
createMessageForReceipt после упрощения будет возвращать объект, который легко протестировать.
Функция convertToReceiptJmsResponse
Во-первых, mqConfig передавать не нужно, это — параметр класса, его класс получает в виде инъекции. Во-вторых, бизнес-логику конвертации стоит выносить из сервиса в класс, так как в данном приватном методе её не протестировать наглядно и просто.
Функция convertToBasicJmsResponse
Те же замечания что и к предыдущей функции.
Функция prepareSipHeaders
Те же замечания что и к предыдущей функции.
Функция processMqResponse
В HttpResponseEnvelope.emerge нет умного конструктора, поэтому функцию назвать нужно было map и использовать просто в of.
Кроме того, нужно правильно применить Result в классе HttpResponseEnvelope. Сейчас, этот метод (функция) возвращает Result просто потому что.
Функция submitMqReceipt
Судя по развилке, функция делает несколько дел сразу.
- Если на вход залетает mqResponse — Result<MqResponseMessage> в виде Result.success — то функция некрасиво создает квитанцию createMessageForReceipt. И через две операции мэпит в JMS ответ и отправляет в MQ.
- Если на вход (строка 204) залетает mqResponse — Result<MqResponseMessage> в виде Result.failure — то создаётся createMessageForReceipt квитанции об ошибке.
К тому же функция submitMqReceipt принимает на вход слишком много лишних параметров.
Функция submitMqResponse чуть проще, но ошибки те же: выбор, конвертация, отправка.
После форматирования чуть проще воспринять.
В следующей части начнём рефакторить код и разбирать всё подробнее.
