Возможности рефакторинга — Code Review Pro

Возможности рефакторинга в Code Review

Самое трудное решение ревьюера — не «надо ли это улучшить», а «надо ли это улучшить в этом PR». Второе решение принимается чаще и ошибается дороже.

#Результат урока

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

#1. Главное решение: сейчас или тикетом

Автор пришёл с фичей и попутно затронул код, который вам не нравится. У вас три варианта, и выбор между ними — навык.

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

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

Не делать вообще. Уместно, когда код скоро удалят, когда он не менялся два года и работает, когда выгода умозрительна. Не всякий несовершенный код — долг: долг это то, что мешает.

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

Проверьте себя. Посмотрите на последнее ваше требование рефакторинга в чужом PR. Стало бы дороже, если бы вы оформили его тикетом?

Частая ошибка. Использовать чужой PR как повод исправить давно раздражающий код. Автор фичи не обязан платить за долг, который накопили до него.

#2. Рефакторинг и поведение не смешиваются

Правило, которое стоит защищать жёстко: в одном коммите либо меняется структура при неизменном поведении, либо поведение при неизменной структуре.

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

Второе следствие — откат. Если фича сломалась, а в том же коммите переехали двадцать файлов, вернуть только фичу невозможно.

❌ один PR: «Рефакторинг платежей + поддержка СБП»
   1200 строк, 40 файлов, в середине изменена логика повторов

✅ два PR:
   1) «Выделен PaymentGateway из PaymentService» — поведение не меняется,
      тесты те же и все зелёные
   2) «Добавлен провайдер СБП» — 80 строк, видно всё

Формулировка замечания: «раздели, пожалуйста, — сейчас я не могу отличить перенос кода от изменения логики». Это не придирка к процессу, а прямое утверждение о том, что ревью невозможно.

Признак в диффе, по которому это ловится: файлов много, строк много, а тесты не изменились ни в одном. Либо поведение действительно не менялось — и тогда это чистый рефакторинг, который надо было выделить, — либо менялось, но без теста.

Проверьте себя. Найдите в истории проекта коммит на 500+ строк со словом «рефакторинг» в названии и проверьте, менялось ли в нём поведение.

Частая ошибка. Соглашаться на смешанный PR, потому что «уже написано». Цена — ненайденный баг, и она выше цены разделения.

#3. Code smells, которые действительно стоят замечания

Каталог запахов велик; в ревью полезны те, у которых есть измеримое следствие.

Дублирование с расхождением. Проблема не в двух похожих функциях, а в том, что при правке исправят одну. Признак срочности: дубли уже разошлись — значит, баг живёт в одной из копий.

# в двух местах, и в одном уже забыли про скидку
def total_for_invoice(order):  return sum(i.price * i.qty for i in order.items)
def total_for_email(order):    return sum(i.price * i.qty for i in order.items) - order.discount

Здесь замечание обязательно: пользователь видит в письме одну сумму, в счёте другую.

Длинная функция. Само число строк — слабый аргумент. Сильный: невозможность протестировать часть логики отдельно. «Правило начисления нельзя проверить, не подняв базу» — проверяемое утверждение, «функция на 80 строк» — нет.

Флаг-параметр. def send(user, is_admin=False), внутри которого две несвязанные ветки, — это две функции, склеенные булевым переключателем. Каждый вызов заставляет читателя идти внутрь.

Завистливая функция. Метод, который обращается к полям другого объекта чаще, чем к своим, — обычно живёт не в том классе.

Цепочка обращений. order.user.company.billing.address.city — знание о структуре пяти классов в одной строке. Изменение любого из них ломает это место.

Магические числа и хардкод. if user_id == 42: в бизнес-логике — обычно временное решение, ставшее постоянным. if status == 3 требует чтения таблицы, чтобы понять код.

Проверьте себя. Найдите в проекте два разошедшихся дубля расчёта и определите, какой из них правильный.

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

#4. Технический долг как решение, а не грех

Долг сам по себе нормален: иногда быстрое решение — правильный выбор, потому что важнее выйти к сроку. Ненормально другое — необъявленный долг.

Полезная формулировка в ревью: не «перепиши», а «зафиксируй». Комментарий в коде со ссылкой на тикет, строка в описании PR, запись в реестре долга — что угодно, лишь бы решение было видимым.

# Считаем последовательно: на текущих объёмах это 200 мс.
# Пакетный расчёт — PROJ-1841, делать при росте каталога выше 5000 позиций.
for product in products:
    ...

Такой комментарий стоит дороже, чем кажется: он сообщает следующему разработчику, что автор знал об ограничении, назвал порог и не забыл. Без него через год кто-то потратит день, выясняя, было это осознанно или недосмотр.

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

Проверьте себя. Есть ли в проекте место, о котором «все знают, что там плохо», но нет ни тикета, ни комментария?

Частая ошибка. Вести реестр технического долга отдельно от кода. Никто не сверяется со списком, зато комментарий рядом с кодом видят все.

#5. Правило скаута и его границы

«Оставь код чище, чем нашёл» работает, пока улучшение попадает в те же строки, которые вы и так меняете. Тогда оно не удорожает ревью.

Граница проходит там, где чистка начинает выходить за пределы задачи. Переименовать переменную в функции, которую правите, — да. Переформатировать файл целиком — нет: дифф раздувается, история git blame затирается, и все строки файла начинают указывать на вас вместо авторов настоящих изменений.

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

Проверьте себя. Запустите git blame на файле, который недавно переформатировали. Видно ли в нём авторов содержательных изменений?

Частая ошибка. Автоформатирование всего файла в PR с фичей. Полезное действие, выполненное в неподходящий момент.

#6. Разбор: PR «Поддержка нескольких валют + рефакторинг расчётов»

Название PR уже содержит проблему. 640 строк, 22 файла.

 # billing/calculator.py
-def calc(order):
-    t = 0
-    for i in order.items:
-        t += i.price * i.qty
-    if order.promo:
-        t = t - t * order.promo.percent / 100
-    if t > 5000:
-        ship = 0
-    else:
-        ship = 300
-    return t + ship
+class PriceCalculator:
+    def __init__(self, rates: RateProvider):
+        self._rates = rates
+
+    def calculate(self, order: Order, currency: Currency = Currency.RUB) -> Money:
+        subtotal = self._subtotal(order)
+        discounted = self._apply_promo(subtotal, order.promo)
+        shipping = self._shipping(discounted)
+        total = discounted + shipping
+        return self._convert(total, currency)
+
+    def _subtotal(self, order: Order) -> Money:
+        return sum((Money(i.price, Currency.RUB) * i.qty for i in order.items),
+                   start=Money.zero(Currency.RUB))
+
+    def _apply_promo(self, amount: Money, promo: Promo | None) -> Money:
+        if promo is None:
+            return amount
+        return amount * (Decimal(100 - promo.percent) / 100)
+
+    def _shipping(self, amount: Money) -> Money:
+        if amount >= Money(5000, Currency.RUB):
+            return Money.zero(Currency.RUB)
+        return Money(300, Currency.RUB)
+
+    def _convert(self, amount: Money, currency: Currency) -> Money:
+        if currency == amount.currency:
+            return amount
+        return amount * self._rates.get(amount.currency, currency)

Плюс: новый класс Money, замена float на Decimal в шести моделях, обновление 14 вызовов, изменение схемы API.

Что видит ревьюер. Рефакторинг здесь качественный — введение типа Money вместо float для денег правильно, разбиение расчёта на шаги читается лучше. Проблема не в качестве, а в том, что найти внутри 640 строк изменение поведения невозможно.

А оно есть, и не одно. В старом коде порог бесплатной доставки проверялся как t > 5000; в новом — amount >= Money(5000). Это исправление off-by-one, то есть изменение поведения, и оно ничем не обозначено. Возможно, автор исправил его намеренно, возможно — случайно, когда переписывал. Из PR понять нельзя.

Второе: в старом коде скидка применялась к сумме товаров, доставка добавлялась после. В новом порядок тот же — но проверить это утверждение можно только прочитав оба варианта целиком и построив таблицу. При 640 строках диффа ревьюер этого не сделает.

Третье: переход с float на Decimal в моделях меняет формат сериализации в API — то есть ломающее изменение для клиентов, спрятанное внутри PR про рефакторинг.

И тесты: изменены четыре файла тестов. Если рефакторинг не меняет поведение, тесты меняться не должны — их правка означает либо изменение поведения, либо тесты были привязаны к реализации.

Комментарии в PR:

PR в целом · blocker Здесь три разных изменения в одном PR: рефакторинг расчёта (поведение не меняется), введение Money и Decimal (меняет формат API) и поддержка валют (новая функциональность). В таком виде ревью невозможно: я не могу отличить перенос кода от изменения логики — и уже нашёл одно незаявленное изменение поведения, см. ниже.

Предлагаю разделить на три PR в этом порядке: 1) Money/Decimal в моделях с переходным периодом в API, 2) чистый рефакторинг расчёта с теми же тестами, 3) поддержка валют. Каждый будет обозрим, и любой можно откатить отдельно.

billing/calculator.py:26 · blocker Было t > 5000, стало amount >= Money(5000). Это изменение поведения: заказ ровно на 5000 ₽ раньше платил доставку, теперь нет. Похоже на исправление реального бага — но тогда оно должно быть отдельным PR с тестом на границу и отметкой в changelog, потому что это влияет на деньги. Если правка случайная — тем важнее её выделить.

models.py:12 · blocker float → Decimal в моделях меняет сериализацию: клиенты получат "1234.56" вместо 1234.56. Мобильное приложение делает арифметику с этими значениями. Это ломающее изменение, и внутри PR про рефакторинг его никто не заметит.

tests/test_calculator.py · major Тесты изменены в четырёх файлах. Если рефакторинг сохраняет поведение, тесты должны остаться прежними и остаться зелёными — это единственное доказательство сохранения поведения. Если их пришлось править, значит либо поведение изменилось, либо они проверяли реализацию. Давайте разберёмся, какой из двух случаев.

billing/calculator.py:14 · praise Money с валютой в типе — хорошее решение: складывать рубли с долларами теперь нельзя по построению. Это то улучшение, за которое стоит потратить отдельный PR.

Чем закончилось. Автор разделил работу на три PR. Во втором, чисто рефакторинговом, тесты действительно остались без изменений и прошли — что и подтвердило сохранение поведения.

Изменение с > на >= оказалось случайным: автор переписывал условие и «поправил как правильнее». Это выяснилось только благодаря разделению. Проверили в поддержке — жалобы на доставку при заказе ровно на 5000 ₽ действительно были, то есть баг настоящий. Исправление выпустили отдельным PR с тестом на границу и отметкой в changelog, поскольку оно влияет на суммы в счетах.

Именно этот случай — лучший аргумент в пользу правила: изменение, влияющее на деньги, было бы выпущено молча внутри 640 строк «рефакторинга».

#7. Чек-лист

РАЗДЕЛЕНИЕ  структура и поведение не смешаны в одном PR
ДОКАЗАТЕЛЬСТВО чистый рефакторинг — тесты не изменились и зелёные
ОБЪЁМ       PR обозрим; уборка не выходит за пределы затронутых строк
СРОЧНОСТЬ   для каждого требования назван ответ на «станет ли дороже потом»
ДУБЛИ       устраняются те, что разошлись или разойдутся; случайно похожие — нет
ДОЛГ        осознанный компромисс зафиксирован комментарием со ссылкой на тикет
ГРАНИЦА     не требуем от автора фичи гасить долг, накопленный до него
ИСТОРИЯ     нет переформатирования целых файлов вместе с содержательной правкой

#Что дальше

Как отличить нужную абстракцию от преждевременной — Паттерны проектирования. Чем измерять то, что обсуждалось качественно, — Метрики качества кода. Что можно отдать инструментам — Автоматизация.

Ключевая мысль: главная ценность разделения рефакторинга и поведения — не чистота истории, а то, что изменение логики становится видимым. В смешанном PR его не находит никто.