Семь признаков, указывающих на необходимость пересмотра и предвещающих дорогостоящие изменения в будущем

Большинство проблематичного кода не выглядит поврежденным. Он компилируется, проходит тесты и нормально работает в демо-версии, однако опытные рецензенты часто могут уже через минуту понять, что следующие изменения, инциденты или передача кода принесут вред. В этом руководстве рассматриваются семь признаков, на которые они реагируют, объясняется, что предвещает каждый из них, и показано, как отличить настоящую проблему от ситуации, которая лишь кажется подозрительной, чтобы вы могли применять тот же подход при своих рецензиях.

Что на самом деле говорит вам «запах кода»

Опытный рецензент редко нуждается в прочтении всего модуля, чтобы заметить проблемы. Булевый аргумент, назначение которого неясно в месте вызова функции, утилита с дюжиной необязательных функций, блок catch, преобразующий каждую ошибку в одинаковое на первый взгляд безобидное значение — ничто из этого сегодня не нарушает работу системы. Проблема заключается в том, что потребуют эти элементы завтра.

Этот инстинкт проистекает из опыта, а не из более строгих руководств по стилю. Люди, которые годами обслуживают системы, видели, как одно копируемое правило превращалось в три противоречивые версии, как удобная абстракция становилась местом решения каждого особого случая, а параметр по умолчанию скрывал сбои в производстве на недели. Они научились распознавать те места в коде, где скапливается неопределенность.

Полезно рассматривать неприятный запах как сигнал, а не как окончательный вердикт. Он не доказывает, что реализация неверна; он лишь указывает на необходимость более тщательного анализа кода:

• Кто именно или какой модуль несет ответственность за это решение?
• Как это будет работать при изменении требований?
• Будет ли сбой все еще заметен при его возникновении?
• Сможет ли следующий разработчик понять, почему код написан именно так, без необходимости выяснять, кто его написал?

Семь сигналов, перечисленных ниже, наиболее вероятно приведут к убыткам, если эти вопросы не задать заранее.

1. Одна функция выполняет несколько нерелевантных задач

Длинные функции привлекают критику, но длина редко бывает тем фактором, который беспокоит опытного рецензента. Функция может состоять из множества строк, поскольку она выполняет одну четкую операцию в виде последовательности понятных шагов. Другая функция может вместиться в двадцать строк, но при этом оставаться сложной для использования, так как одновременно занимается проверкой входных данных, контролем прав доступа, сохранением в базу данных, форматированием ответа, отправкой уведомлений и обработкой ошибок.

Настоящая проблема заключается в том, что смешанные обязанности дают функции множество независимых причин для изменений. Новое правило доступа влияет на нее, изменение формата ответа также влияет на нее, миграция схемы тоже влияет на нее, появление новых требований к уведомлениям тоже влияет на нее. Проблемы, которые должны решаться отдельно, в итоге сосредотачиваются в одном месте, и даже небольшая правка заставляет человека понимать весь процесс целиком.

Полезным признаком является название, которое не может честно описать функцию. Например, что-то вроде processOrder может проверять информацию клиента, рассчитывать цены, удерживать запасы, создавать интент оплаты, сохранять информацию о заказе и отправлять подтверждение по электронной почте. Название остается неопределенным, потому что точное название заняло бы целый абзац.

Почему механическое извлечение — не решение

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

• Где принимается бизнес-решение?
• Какая часть влияет на внешнюю систему?
• Какая часть представляет собой чистую трансформацию данных?
• Какая часть отвечает за транзакцию?

Как только эти соединения становятся видимыми, код можно организовать так, как на самом деле происходит выполнение операции. Основной проблемой никогда не было количество работы, выполняемой функцией. Проблема заключалась в том, что никто не мог изменить хотя бы одно решение, не учитывая при этом несколько других, не связанных между собой факторов.

2. Логические параметры, включающие скрытые режимы

Логический аргумент кажется безобидным, поскольку содержит всего один бит. На практике он часто скрывает целый альтернативный способ работы под существующим именем функции. Рассмотрим вызов, подобный приведённому ниже, который почти ничего не говорит читателю о том, что произойдёт:

await updateAccount(accountId, true, false);

Чтобы понять его работу, необходимо открыть реализацию и выяснить, принудительно ли первое значение true запускает обновление, инициирует уведомление, обходит проверку валидности, сохраняет историю или активирует учетную запись. Автор функции может это помнить, но все остальные вынуждены восстанавливать смысл на основе позиций аргументов, причем заменена пара литералов сможет пройти любую проверку типов.

Комбинации, которые никто не предусмотрел

Проблема усугубляется по мере накопления флагов. Три логических значения дают восемь возможных комбинаций, некоторые из которых никто даже не рассматривал. Если функция принимает параметры forceUpdate, skipValidation и sendNotification, что должно произойти, когда принудительное обновление обходит проверку валидности, но все равно отправляет уведомление? Система типов с радостью разрешает такой вызов, даже если это не соответствует требованиям бизнес-логики.

Рецензенты отмечают это, поскольку набор флагов обычно означает, что одна функция заменяет несколько операций. Они могут использовать общие внутренние механизмы, но у них разные цели. Изменение контактных данных, проверка аккаунта и применение административного переопределения могут все записываться в одну и ту же строку, однако ни у кого не должно возникать необходимости запоминать, какой шаблон флагов выбирает тот или иной рабочий процесс.

Выбор подходящего рефакторинга

В некоторых случаях достаточно перейти на объект с одним аргументом, свойства которого имеют имена, поскольку updateAccount(id, { notify: true }) хотя бы сам по себе объясняет своё назначение в месте вызова. В других случаях отдельные функции, такие как verifyAccount и overrideAccount, делают бизнес-цель явной и полностью устраняют невозможные комбинации. Выбор подхода зависит от того, являются ли эти действия реальными вариантами одной операции или отдельными операциями, которые случайно используют общий код.

Логические значения не являются по своей природе плохими. Опция с простым и понятным названием, такая как includeArchived, совершенно разумна. Предупреждающим знаком является флаг, который меняет смысл функции, вместо того чтобы изменять какой-то хорошо понятый деталь.

3. Одно и то же бизнес-правило, выраженное в разных формах

Текст, скопированный и вставленный, легко распознать. Однако дублирование знаний опаснее, поскольку каждая копия может выглядеть совершенно иначе.

Возьмем правило разрешения. Оно может проявляться в виде скрытой кнопки в интерфейсе, проверки роли в контроллере, условия статуса в сервисе или клетки WHERE в запросе. Каждый слой, кажется, защищает систему, но каждый из них может содержать немного иное понимание того, кому разрешено действовать.

Всё работает нормально до тех пор, пока не меняется политика. Один конечный пункт принимает новое правило, другой продолжает использовать старую логику, а запланированная задача обходит их оба, напрямую обращаясь к внутреннему сервису. Теперь интерфейс предлагает действие, которое сервер отклоняет. Одна часть приложения считает пользователя авторизованным из-за его роли, в то время как другая настаивает на том, что он должен быть владельцем записи.

Опытные эксперты отмечают повторяющиеся комбинации статусов, ролей, флагов и дат, поскольку такие условия часто означают одно и то же бизнес-понятие, изложенное несколько раз. Они спрашивают, где находится официальное решение, и устанавливают, обеспечивают ли другие слои его соблюдение или тайно переопределяют его.

Слои с разными целями — это нормально

Это не означает, что правило может существовать только в одной строке кода во всей системе. Интерфейс может отражать полномочия для повышения удобства использования, в то время как бэкенд должен обеспечивать их соблюдение ради безопасности. Ограничения базы данных могут защищать ту же неизменную величину на уровне хранения. У этих слоев разные задачи. Проблема возникает, когда каждый из них отвечает на бизнес-вопрос по своим критериям.

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

4. Общий вспомогательный компонент с постоянно расширяющимся объектом опций

Многие проблемные абстракции изначально возникают как разумные вспомогательные элементы. Когда одна и та же логика встречается в двух местах, общая часть извлекается, в результате получается небольшой, сфокусированный и легко понимаемый код.

Затем оба вызывающих начинают требовать разного. Один нуждается в повторных попытках; другой не должен повторять свой запрос. Один из них получает преимущества от кэширования; другой не должен видеть устаревшие значения. Один возвращает полный набор данных; другой передаёт их по потоку. Вместо того чтобы разделять эти рабочие процессы, вспомогательный инструмент создаёт новый параметр для каждой разницы.

Вскоре объект параметров охватывает перевод ошибок, изменение формата результата, настройки кэширования, количество повторных попыток, обработку аутентификации, функции-запускатели и несколько логических переключателей. Большинство вызывающих используют два-три из этих параметров, но любые изменения могут повлиять на все из них.

Это проявление определенного запаха, связанного с тем, что абстракция перестала устранять сложность и начала ее концентрировать. Интерфейсы вызовов кажутся простыми, но понимание системы теперь подразумевает знание того, как каждый вариант взаимодействует со всеми остальными. Большое количество вызовов не является доказательством надежности абстракции; иногда это лишь указывает на то, что использование вспомогательных элементов стало неизбежным.

Похожая синтаксис против общего поведения

Ключевой вопрос заключается в том, делают ли вызовы общее стабильное поведение или имеют ли они лишь похожий на вид код. Две операции могут отправлять HTTP-запросы, но обрабатывать сбои и попытки повтора по-разному. Два формата могут иметь одинаковые поля, но различаться в проверке данных, отправке и обработке черновиков. Их объединение устраняет видимое дублирование, но затрудняет выражение всех реальных различий.

Обычно лучшим решением является введение более узкой абстракции позже. Вместо того чтобы делиться всем рабочим процессом, следует использовать отдельный стабильный элемент — например, одну трансформацию, одну политику или один вызов низкого уровня — и четко выделять отдельные рабочие процессы. Повторное использование по-прежнему ценно; проблемой должна стать ситуация, когда каждое новое требование добавляет еще один вариант, который существующим пользователям не нужен и непонятен. Чтобы узнать о подходе, ориентированном на фронтенд к этой же проблеме, ознакомьтесь с почему абстракции фронтенда тихо превращаются в технический долг.

5. Обработка ошибок, маскирующая неудачу под успех

Блок catch может создавать впечатление надежности приложения, тем самым тайно подрывая доверие к нему.

Представьте слой доступа к данным, который в случае ошибки базы данных возвращает []. Или сервис, у которого сбивается HTTP-запрос, и он просто возвращает null. Фронтенд отображает пустое состояние для каждого отклонённого запроса. Ничего не ломается, но система больше не может отличить настоящего отсутствия данных от сбоя в работе.

Аудиторы быстро замечают эту проблему, потому что знают, как она проявляется во время инцидентов. Панель управления показывает нулевое количество записей, когда база данных не работает. Клиент видит сообщение «Заказов не найдено» после того, как его сессия молча истекла. Фоновая задача отмечает элемент как завершённый, поскольку зависимость не вернула ничего полезного. Интерфейс остаётся спокойным, так как заменил правду на правдоподобный ответ.

Вопросы, которые следует задать к каждому блоку обработки ошибок

При анализе обработки ошибок проверьте три момента:

• Сохраняется ли первоначальная причина, например в качестве cause новой ошибки или, по крайней мере, в логах?
• Может ли вызывающий код отличить ожидаемый бизнес-результат от технической неисправности?
• Принадлежит ли слой, перехватывающий ошибку, ответственность за следующее решение?

Если код на данном уровне не имеет возможности восстановиться, попробовать снова, отменить частичную работу или предоставить разумную замену, перехват ошибки может ничего не дать, кроме как уничтожить доказательства.

Ничто из этого не означает, что сырые исключения должны попадать к пользователям. Сообщения, отображаемые людям, должны быть безопасными и понятными, а внутренние ошибки часто требуют преобразования в стабильные категории домена. Требование заключается в том, чтобы такое преобразование сохраняло достаточно смысла, чтобы как приложение, так и разработчики могли правильно отреагировать.

Пустой результат должен означать, что операция прошла успешно и ничего не было найдено. Неудачная операция должна оставаться неудачной, если только система не определила специально безопасный вариант решения. Слияние этих двух ситуаций ради удобства замедляет отладку и предоставляет пользователям вводящую в заблуждение информацию. Хорошая обработка ошибок позволяет увидеть неудачу; плохая обработка ошибок скрывает её.

6. Небольшое требование с обширными, не связанными между собой изменениями

Опытные рецензенты часто сначала оценивают общую структуру запроса на внесение изменений, прежде чем читать отдельные строки. Когда небольшое требование распространяется на длинный список не связанных между собой файлов, это обычно указывает на скрытую взаимосвязь или на задачу, отклонившуюся от своей цели.

Добавление одного поля может закономерно затронуть механизмы сохранения данных, сервисы, ответы API и пользовательский интерфейс; это целостное изменение, охватывающее все уровни системы. Проблема возникает, когда один и тот же pull request также переписывает общие утилиты, меняет названия несвязанных типов, изменяет обработку ошибок, перемещает модули и обновляет крупные файлы с копиями данных.

Каждая из этих правок может считаться улучшением, но вместе они объединяют несколько причин изменений в одну единицу. Рецензенты должны одновременно оценивать новую функциональность, рефакторинг, изменения в интерфейсах и процедуры очистки кода. Если происходит регрессия, становится сложнее определить конкретную причину. Если функциональность приходится откатывать, вместе с ней уходят и несвязанные улучшения.

Почему кодирование с помощью ИИ делает это более распространенным

Инструменты генерации кода способствуют тому, что сотрудники чаще попадают в эту ловушку, поскольку полная переработка кода стоит практически ничего. Если запросить исправление всего одной ошибки, можно получить отлаженную заменяющую функцию с новыми именами, иным порядком выполнения операций, совершенно новыми абстракциями и изменённым поведением при возникновении ошибок. Создание такой функции стоит почти ничего; однако её проверка всё равно требует от команды тех же усилий на анализ, что и раньше.

Хорошим правилом является соблюдение пропорции между размером изменения и степенью его понятности при объяснении. Узкая диагностика обычно требует узкого решения, а более масштабные изменения можно включить в отдельный pull request, где их можно рассмотреть и протестировать с учетом их собственных особенностей. Это не означает требования создавать мелкие pull request во всех случаях; некоторые архитектурные изменения по своей природе являются масштабными. Вопрос заключается в том, служит ли каждое значимое изменение одной целостной цели. Когда разница между исходным и измененным кодом превышает размер самой проблемы, обычно одновременно решаются несколько вопросов без их разделения, что затрудняет выявление причин и следствий.

7. Код, который работает, но затрудняет проверку

Некоторые реализации пишутся быстро именно потому, что их поведение трудно наблюдать или контролировать. Функция считывает данные системных часов напрямую, формирует собственные зависимости, изменяет глобальное состояние, выполняет несколько внешних вызовов, скрывает собственные ошибки и почти ничего не возвращает. В случае успеха всё может быть в порядке, но демонстрация сбоев оказывается сложной и трудоемкой.

Степень тестируемости обычно отражает качество границ. Если правило бизнес-логики невозможно протестировать без запуска всего приложения, загрузки данных в базу, вызова реальных сервисов и ожидания реальных таймеров, это правило, скорее всего, связано с слишком большим количеством побочных эффектов.

Асинхронные рабочие процессы выявляют этот разрыв

Проблема наиболее остра в многократных асинхронных процессах. Представьте функцию, которая сохраняет изменения, генерирует событие домена, уведомляет пользователя и добавляет строку в журнал аудита. Если третий шаг сбоит, может ли тест определить, какие шаги уже были выполнены? Можно ли заставить зависимость истечь по времени? Можно ли симулировать двойное выполнение функции? Можно ли воспроизвести устаревшее обновление, не полагаясь на случайности времени?

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

Проверяйте на уровне, где существует риск

Чистота во всем и использование моков для всего тоже не являются решением. Тесты, построенные в основном на моках, склонны зафиксировать внутренние детали на месте, игнорируя при этом реальное поведение базы данных, сети или транзакций. Лучший подход — определить, какие гарантии важны, и проверять каждую из них там, где на самом деле существует риск:

• Чистые преобразования можно тестировать в изоляции с помощью модульных тестов.
• Для проверки инвариантов базы данных часто требуются тесты интеграции с реальной базой данных.
• Для определения границ сервисов могут понадобиться тесты контрактов.
• Распределенное поведение может зависеть от структурированных логов и идентификаторов корреляции, поскольку не каждую условие производства можно точно воспроизвести.

Использование специальных механизмов для управления временем выполнения или зависимостями вместо обращения к глобальным переменным часто является наименьшей модификацией, позволяющей контролировать важные решения. Проблема заключается не столько в низком уровне покрытия тестами, сколько в коде, в котором ключевые решения невозможно контролировать, отслеживать или объяснять, когда «идеальный» сценарий выполнения перестает срабатывать.

Основные выводы

Ни один из этих признаков не является автоматическим вердиктом. Длинная функция может представлять собой одну четкую операцию, логическая переменная может быть совершенно очевидной, общий вспомогательный модуль может обеспечивать действительно стабильное поведение, а значительные изменения могут быть неизбежными. Рассматривайте этот список как способ определения того, где нужно углубиться в анализ, а не как чек-лист для механического соблюдения всех пунктов.

При анализе важны следующие вопросы:

• Не смешаны ли обязанности внутри функции, из-за чего одно изменение влечет за собой нерелевантные решения?
• Могут ли пользователи понять функцию без необходимости расшифровки специальных флагов?

У каждого бизнес-правила есть четкий ответственный?

Не скрывает ли общая абстракция реальные различия за опциями?

Сохраняет ли обработка ошибок информацию о том, что пошло не так?

Является ли разница пропорциональной требованию, которое она обслуживает?

Можно ли проверить важное поведение при возникновении проблем?

Опытные рецензенты на самом деле выявляют отложенную неопределенность: вероятность того, что следующее требование заставит кого-то понимать не связанную логику, что следующий инцидент оставит слишком мало улик для расследования, что следующий разработчик не будет знать, какое поведение было намеренным, или что следующий пользователь добавит еще один вариант к абстракции, которая уже несет слишком большую нагрузку. Они быстро распознают эти признаки, потому что уже сталкивались с ними ранее: приходилось разбираться с перегруженными вспомогательными функциями, отслеживать поглощенные исключения, согласовывать противоречивые правила и рассматривать «небольшие» функции, затрагивающие половину кодовой базы. Цель не в том, чтобы устранить код, который выглядит неряшливо; цель — обнаружить код, который сделает будущую работу более сложной, чем это необходимо.