Как не стоит писать код: разбираем ошибки
Вторая часть цикла статей про чистый код. В ней покажем пример некачественного кода и разберём основные ошибки.
6К открытий12К показов
В прошлой части мы рассказали, что такое чистый код и какие принципы нужно соблюдать, чтобы его написать. В новой — поговорим про плохой код: перечислим проблемы и покажем, как их решать.
Эта статья — «прожарка». Она не отвечает на все вопросы и не содержит иллюстрации ко всей критике. В будущем мы шаг за шагом более атомарно будем всё разгребать.
Максим Морев
Software Craftsmanship энтузиаст, CTO
Ваганов Вадим
Software Engineer, Head of Profession backend-разработки
Сперва «высечем» тезисы: как нельзя писать
Всё, рассказанное ниже, база, которую мы взяли из своего опыта, и теперь хотим поделиться.
- НЕ пишите, если не нравится — круто, если код вас увлекает, и время в компании 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 чуть проще, но ошибки те же: выбор, конвертация, отправка.
После форматирования чуть проще воспринять.
В следующей части начнём рефакторить код и разбирать всё подробнее.
6К открытий12К показов