Как не стоит писать код: разбираем ошибки

В прошлой части мы рассказали, что такое чистый код и какие принципы нужно соблюдать, чтобы его написать. В новой — поговорим про плохой код: перечислим проблемы и покажем, как их решать.

Эта статья — «прожарка». Она не отвечает на все вопросы и не содержит иллюстрации ко всей критике. В будущем мы шаг за шагом более атомарно будем всё разгребать.

Максим Морев

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 чуть проще, но ошибки те же: выбор, конвертация, отправка.

После форматирования чуть проще воспринять.

В следующей части начнём рефакторить код и разбирать всё подробнее.