Code Review And Quality

Ревью кода и качество

Обзор

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

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

Когда применять

• Перед вливанием любого пулл-реквеста или изменения
• После завершения реализации фичи
• Когда другой агент или модель произвели код, который нужно оценить
• При рефакторинге существующего кода
• После любого багфикса (ревьюишь и исправление, и регрессионный тест)

Ревью по пяти осям

Каждое ревью оценивает код по этим измерениям:

1. Корректность

Делает ли код то, что заявляет?

• Соответствует ли он спеке или требованиям задачи?
• Обработаны ли краевые случаи (null, пустое значение, граничные значения)?
• Обработаны ли пути с ошибками (а не только счастливый путь)?
• Проходят ли все тесты? А тесты действительно проверяют то, что нужно?
• Нет ли ошибок на единицу, гонок или рассогласований состояния?

2. Читаемость и простота

Поймёт ли этот код другой инженер (или агент) без объяснений автора?

• Описательны ли имена и согласованы ли они с соглашениями проекта? (Никаких temp, data, result без контекста)
• Прямолинеен ли поток управления (без вложенных тернарников и глубоких колбэков)?
• Логично ли организован код (связанное — рядом, границы модулей ясны)?
• Нет ли «умных» трюков, которые стоит упростить?
• Можно ли сделать это меньшим числом строк? (1000 строк там, где хватает 100, — это провал)
• Оправдывают ли абстракции свою сложность? (Не обобщай раньше третьего случая применения)
• Помогли бы комментарии прояснить неочевидный замысел? (Но не комментируй очевидный код.)
• Нет ли следов мёртвого кода: переменных-пустышек (_unused), прокладок для обратной совместимости, комментариев // removed?
• Не прикручено ли новое условие к постороннему потоку? Это запах архитектуры, а не мелочь: вынеси логику в отдельный хелпер, состояние или политику, а не запутывай существующий путь.
• Не появляются ли повторяющиеся условия на одной и той же форме данных? Это признак недостающей модели или диспетчера. «Временная» ветка обычно становится постоянным долгом.

3. Архитектура

Вписывается ли изменение в устройство системы?

• Следует ли оно существующим паттернам или вводит новый? Если новый — обоснован ли он?
• Сохраняет ли оно чистые границы модулей?
• Нет ли дублирования кода, которое стоило бы вынести в общее?
• Направлены ли зависимости в правильную сторону (никаких циклов)?
• Уместен ли уровень абстракции (не переусложнено, не переплетено)?
• Этот рефакторинг снижает сложность или просто перекладывает её? Посчитай, сколько понятий читатель должен держать в голове, чтобы понять изменение. Если «более чистая» версия оставляет это число прежним — она не чище. Предпочитай перестройку, после которой исчезают целые ветки, режимы или слои, той, что просто заново централизует ту же логику. Удалить абстракцию лучше, чем отполировать её.
• Не протекает ли логика конкретной фичи в общий модуль? Держи логику в её слое, переиспользуй существующий канонический хелпер вместо почти-дубликата и не узаконивай архитектурный дрейф.
• Явные ли границы типов? Ставь под вопрос беспричинные any/unknown/необязательные поля/приведения типов и молчаливые запасные значения, замазывающие неясный инвариант: сделав границу явной, ты часто упрощаешь и окружающий поток управления.

4. Безопасность

Подробное руководство по безопасности — см. security-and-hardening. Вносит ли изменение уязвимости?

• Валидируется и очищается ли пользовательский ввод?
• Держатся ли секреты вне кода, логов и системы контроля версий?
• Проверяются ли аутентификация и авторизация там, где нужно?
• Параметризованы ли SQL-запросы (без склейки строк)?
• Кодируется ли вывод, чтобы предотвратить XSS?
• Из доверенных ли источников зависимости и нет ли у них известных уязвимостей?
• Считаются ли недоверенными данные из внешних источников (API, логи, пользовательский контент, конфиги)?
• Валидируются ли внешние потоки данных на границах системы до использования в логике или отрисовке?

5. Производительность

Подробности профилирования и оптимизации — см. performance-optimization. Вносит ли изменение проблемы с производительностью?

• Нет ли паттернов N+1 в запросах?
• Нет ли неограниченных циклов или неограниченной выборки данных?
• Нет ли синхронных операций, которые должны быть асинхронными?
• Нет ли лишних перерисовок в UI-компонентах?
• Не забыта ли пагинация на эндпоинтах со списками?
• Не создаются ли крупные объекты на горячих путях?

Структурные средства исправления

Когда отмечаешь структурную проблему — предлагай ход, а не только проблему. Ревью, которое говорит лишь «это сложно», оставляет автора гадать. Тянись к названному приёму перестройки:

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

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

Размер изменения

Мелкие сфокусированные изменения легче ревьюить, быстрее вливать и безопаснее выкатывать. Целься в эти размеры:

~100 изменённых строк   → Хорошо. Ревьюится за один присест.
~300 изменённых строк   → Приемлемо, если это одно логическое изменение.
~1000 изменённых строк  → Слишком много. Разбивай.

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

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

Стратегии разбиения, когда изменение слишком велико:

Когда крупные изменения приемлемы: полное удаление файлов и автоматизированный рефакторинг, где ревьюеру нужно проверить замысел, а не каждую строку.

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

Описания изменений

Каждому изменению нужно описание, которое самодостаточно в истории версий.

Первая строка: короткая, в повелительном наклонении, самодостаточная. «Удалить FizzBuzz RPC», а не «Удаление FizzBuzz RPC». Должна быть достаточно информативной, чтобы человек, ищущий по истории, понял изменение, не читая дифф.

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

Антипаттерны: «Фикс бага», «Починил сборку», «Добавил патч», «Перенос кода из A в B», «Фаза 1», «Добавлены вспомогательные функции».

Процесс ревью

Шаг 1: пойми контекст

Прежде чем смотреть код, пойми замысел:

- Чего это изменение пытается добиться?
- Какую спеку или задачу оно реализует?
- Какое изменение поведения ожидается?

Шаг 2: сначала смотри тесты

Тесты раскрывают замысел и покрытие:

- Есть ли тесты на это изменение?
- Проверяют ли они поведение (а не детали реализации)?
- Покрыты ли краевые случаи?
- Описательные ли у тестов имена?
- Поймают ли тесты регрессию, если код изменится?

Шаг 3: смотри реализацию

Пройди по коду, держа в голове пять осей:

Для каждого изменённого файла:
1. Корректность: делает ли этот код то, что говорит тест?
2. Читаемость: понимаю ли я это без посторонней помощи?
3. Архитектура: вписывается ли это в систему?
4. Безопасность: есть ли уязвимости?
5. Производительность: есть ли узкие места?

Шаг 4: категоризируй находки

Помечай каждый комментарий его серьёзностью, чтобы автор понимал, что обязательно, а что нет:

Это не даёт авторам воспринимать всю обратную связь как обязательную и тратить время на необязательные предложения.

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

Шаг 5: проверь саму проверку

Оцени, как автор доказывает работоспособность:

- Какие тесты были прогнаны?
- Прошла ли сборка?
- Проверялось ли изменение вручную?
- Есть ли скриншоты для изменений UI?
- Есть ли сравнение «до/после»?

Паттерн ревью несколькими моделями

Используй разные модели для разных углов зрения:

Модель A пишет код
    │
    ▼
Модель B ревьюит на корректность и архитектуру
    │
    ▼
Модель A отрабатывает замечания
    │
    ▼
Человек принимает окончательное решение

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

Пример промпта для агента-ревьюера:

Отревьюй это изменение кода на корректность, безопасность и соответствие
нашим проектным соглашениям. В спеке сказано [X]. Изменение должно [Y].
Помечай проблемы как Критично, Обязательно, Необязательно или Мелочь.

Гигиена мёртвого кода

После любого рефакторинга или изменения реализации проверь, не осиротел ли код:

1. Найди код, который стал недостижимым или неиспользуемым
2. Выпиши его явным списком
3. Спроси перед удалением: «Убрать эти ставшие неиспользуемыми элементы: [список]?»

Не оставляй мёртвый код валяться — он путает будущих читателей и агентов. Но и не удаляй молча то, в чём не уверен. Сомневаешься — спрашивай.

НАЙДЕН МЁРТВЫЙ КОД:
- formatLegacyDate() в src/utils/date.ts — заменена на formatDate()
- Компонент OldTaskCard в src/components/ — заменён на TaskCard
- Константа LEGACY_API_URL в src/config.ts — ссылок не осталось
→ Можно это удалить?

Скорость ревью

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

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

Разрешение разногласий

Когда разбираешь спор в ревью, применяй эту иерархию:

1. Технические факты и данные важнее мнений и предпочтений
2. Гайд по стилю — абсолютный авторитет в вопросах стиля
3. Проектирование ПО оценивается по инженерным принципам, а не по личным вкусам
4. Согласованность с кодовой базой допустима, если она не ухудшает общее здоровье

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

Честность в ревью

Когда ревьюишь код — свой, другого агента или человека:

• Не штампуй одобрения. «LGTM» без следов реального ревью не помогает никому.
• Не смягчай реальные проблемы. «Возможно, это небольшое замечание» про баг, который дойдёт до продакшна, — это нечестно.
• По возможности переводи проблемы в цифры. «Этот N+1-запрос добавит ~50 мс на каждый элемент списка» лучше, чем «это может тормозить».
• Возражай подходам с явными проблемами. Угодничество в ревью — способ провалиться. Если у реализации есть проблемы, скажи прямо и предложи альтернативы.
• Спокойно принимай, когда тебя переспорили. Если у автора полный контекст и он не согласен — доверься его суждению. Комментируй код, а не людей: переформулируй личную критику так, чтобы она касалась самого кода.

Дисциплина зависимостей

Ревью зависимостей — часть ревью кода:

Прежде чем добавлять любую зависимость:

1. Решает ли это существующий стек? (Часто да.)
2. Насколько зависимость большая? (Проверь влияние на размер бандла.)
3. Активно ли она поддерживается? (Посмотри последний коммит, открытые issue.)
4. Есть ли у неё известные уязвимости? (npm audit)
5. Какая лицензия? (Должна быть совместима с проектом.)

Правило: предпочитай стандартную библиотеку и существующие утилиты новым зависимостям. Каждая зависимость — это обуза.

Обновление существующей зависимости — такое же изменение кода, как любое другое, и самые рискованные обновления те, что влиты пачкой с сообщением вроде «bump deps». Ревьюй их с той же дисциплиной:

1. Читай changelog, а не только номер версии. Semver — это обещание, которое сопровождающий мог и не сдержать: «патч» способен нести изменение поведения. Для мажорного обновления прочитай заметки о миграции и найди, что ломается.
2. По одной зависимости на изменение. Обновляй и вливай их по отдельности (или мелкими связанными группами). Когда пакетное обновление ломает сборку, ты потерял информацию о том, какой пакет виноват; изменение на один пакет делает причину очевидной, а откат — чистым.
3. Пусть решают тесты. Обновление подтверждается зелёным набором до и после, а не тем, что «оно установилось». Если покрытие вокруг поведения этой зависимости тонкое, эта дыра и есть настоящая находка — сначала добавь тест.
4. Помни про транзитивный граф. Большинство установленных пакетов никто не выбирал напрямую. Ревьюй дифф lock-файла, а не только package.json: одно прямое обновление может притащить десятки косвенных изменений.
5. Держи lock-файл честным. Коммить его, ревьюй его дифф и никогда не правь руками. Именно lock-файл на самом деле фиксирует то, что уезжает в прод.

Разбор находок npm audit и рисков цепочки поставок (тайпсквоттинг, скомпрометированные сопровождающие) — по скиллу security-and-hardening: этот раздел про рабочий процесс обновления, тот — про вердикт по безопасности.

Чеклист ревью

## Ревью: [Заголовок PR/изменения]

### Контекст
- [ ] Я понимаю, что делает это изменение и зачем

### Корректность
- [ ] Изменение соответствует спеке/требованиям задачи
- [ ] Краевые случаи обработаны
- [ ] Пути с ошибками обработаны
- [ ] Тесты адекватно покрывают изменение

### Читаемость
- [ ] Имена понятны и согласованы
- [ ] Логика прямолинейна
- [ ] Нет лишней сложности

### Архитектура
- [ ] Следует существующим паттернам
- [ ] Нет лишней связанности и лишних зависимостей
- [ ] Уместный уровень абстракции
- [ ] Рефакторинг снижает сложность, а не перекладывает её
- [ ] В общих модулях нет логики конкретных фич; файл остаётся в здоровых размерах

### Безопасность
- [ ] В коде нет секретов
- [ ] Ввод валидируется на границах
- [ ] Нет уязвимостей к инъекциям
- [ ] Проверки доступа на месте
- [ ] Внешние источники данных считаются недоверенными

### Производительность
- [ ] Нет паттернов N+1
- [ ] Нет неограниченных операций
- [ ] На эндпоинтах со списками есть пагинация

### Проверка
- [ ] Тесты проходят
- [ ] Сборка успешна
- [ ] Ручная проверка выполнена (если применимо)

### Вердикт
- [ ] **Одобрить** — готово к вливанию
- [ ] **Запросить изменения** — проблемы должны быть устранены

См. также

• Подробное руководство по ревью безопасности — ../../references/security-checklist.md
• Проверки производительности при ревью — ../../references/performance-checklist.md

Типовые самооправдания

Тревожные признаки

• Пулл-реквесты вливаются вообще без ревью
• Ревью, которое проверяет только, проходят ли тесты (игнорируя остальные оси)
• «LGTM» без следов реального ревью
• Изменения, чувствительные к безопасности, без ревью с фокусом на безопасность
• Большие PR, которые «слишком велики, чтобы нормально отревьюить» (разбивай их)
• Багфиксы без регрессионных тестов
• Комментарии ревью без пометок серьёзности — непонятно, что обязательно, а что нет
• Принятие «потом починю» — этого не происходит
• Рефакторинг, который перекладывает код с места на место, не сокращая число понятий, удерживаемых читателем
• Изменение, которое растит и без того большой файл вместо декомпозиции
• Новые условия, разбросанные по посторонним путям выполнения (признак недостающей абстракции)
• Самодельный хелпер, дублирующий существующий канонический, или логика фичи, помещённая в общий модуль
• Пакетный PR «bump dependencies» без разбора changelog и без изоляции по пакетам
• Изменение lock-файла, правленное руками, незакоммиченное или влитое без ревью его диффа

Проверка

После завершения ревью:

• Все критичные проблемы устранены
• Все обязательные (без префикса) изменения устранены или явно отложены с обоснованием
• Тесты проходят
• Сборка успешна
• Задокументировано, как всё проверялось (что изменилось, как это подтверждено)
• Обновления зависимостей отревьюены по их changelog, изолированы по пакетам и подтверждены зелёным набором тестов, дифф lock-файла просмотрен

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