Code Review: как делать правильно

Превращаем code review из формальности в инструмент роста команды

Код-ревью — одна из тех практик, которые легко внедрить формально и почти невозможно внедрить правильно. В большинстве компаний процесс выглядит так: разработчик открывает pull request, коллеги ставят «LGTM» через 10 минут, изменения уходят в main. Пользы — ноль. В лучшем случае вы поймаете опечатку в комментарии. В худшем — создадите иллюзию контроля качества там, где его нет. В 404gen мы прошли через несколько итераций выстраивания процесса ревью. На проектах с бюджетом от 3 млн ₽ цена архитектурной ошибки, пропущенной на этапе ревью, исчисляется неделями рефакторинга. Эта статья — выжимка из того, что реально работает: принципы, антипаттерны, конкретные техники и примеры кода. Зачем вообще нужен код-ревью: цели, которые нужно формализовать Большинство команд не могут ответить на простой вопрос: «Чего мы хотим достичь через ревью?» Отсутствие ответа — главная причина того, что процесс превращается в формальность. Код-ревью решает несколько задач одновременно, и важно понимать приоритет каждой: Поиск дефектов до попадания в продакшн. Исследование IBM показало, что исправление бага на этапе ревью обходится в 6–10 раз дешевле, чем после деплоя. На стадии эксплуатации — в 100 раз дороже. Распространение знаний внутри команды. Ревью — единственный систематический способ сделать так, чтобы более одного человека понимал каждый модуль системы. Это страховка от «автобусного фактора». Поддержание архитектурной целостности. Небольшие «технические долги», которые кажутся безобидными в изоляции, накапливаются в системные проблемы. Ревью — точка контроля. Рост Junior-разработчиков. Хорошо написанный review comment стоит больше, чем час на 1-on-1. Выравнивание стиля и конвенций. Наименее важная цель, которую почему-то ставят первой. Для этого существуют линтеры. Когда цели не формализованы, ревьюеры неосознанно скатываются к наиболее простой задаче — проверке стиля — и игнорируют архитектурные проблемы, которые требуют реального погружения. Размер pull request: почему это важнее, чем кажется Исследование SmartBear (анализ более 2000 ревью в Cisco) показало: оптимальный размер PR — от 200 до 400 строк кода. При этом ревьюер способен эффективно работать не более 60–90 минут подряд. Всё, что выходит за эти рамки, резко снижает качество ревью. Закон Хофштадтера в применении к ревью: если PR занимает больше часа на проверку, ревьюер начинает пропускать проблемы — сначала косметические, потом всё более существенные. К 500-й строке внимание падает настолько, что архитектурные ошибки проходят мимо глаз. Практические правила размера PR: Одна задача — один PR. Не «добавил фичу и заодно отрефакторил». Если PR неизбежно большой (например, первоначальная настройка проекта), разбивайте на logical chunks с последовательным ревью. Чисто механические изменения (переименование, форматирование) выносите в отдельный PR без функциональных правок. Используйте правило «could I describe this PR in one sentence?» Если нет — PR нужно разбить. В нашей практике мы ввели мягкий лимит: PR с более чем 600 строками изменений автоматически маркируется меткой «needs-splitting» и возвращается автору до начала ревью. Что проверять: иерархия проблем Ревьюер должен работать по приоритетам сверху вниз, а не замечать то, что бросается в глаза первым. Иерархия проблем выглядит так: Уровень 1: Корректность и безопасность Делает ли код то, что он должен делать? Нет ли уязвимостей? Это — обязательный минимум ревью. Если здесь есть проблемы, останавливаемся и не идём дальше. // Плохо: SQL-инъекция через конкатенацию строк const query = `SELECT * FROM users WHERE email = '${userInput}'`; // Хорошо: параметризованный запрос const query = 'SELECT * FROM users WHERE email = $1'; const result = await db.query(query, [userInput]); Типичные проблемы уровня 1, которые пропускают при поверхностном ревью: Гонки состояний (race conditions) в асинхронном коде Незакрытые соединения и утечки ресурсов Непроверенные входные данные от пользователя Секреты, захардкоженные в коде Обработка граничных случаев (null, пустой массив, INT_MAX) Уровень 2: Архитектура и дизайн Соответствует ли решение принятой архитектуре? Не создаёт ли оно технический долг? Нет ли нарушений SOLID, избыточной связанности? // Плохо: функция делает три вещи и знает о слое представления async function processUserOrder(userId: string, items: Item[]) { const user = await db.getUser(userId); const total = items.reduce((sum, item) => sum + item.price, 0); if (total > user.creditLimit) { showErrorModal('Превышен кредитный лимит'); // UI-логика в бизнес-слое! return; } await db.createOrder({ userId, items, total }); sendEmail(user.email, 'Ваш заказ оформлен'); // Побочный эффект без абстракции analytics.track('order_created', { total }); // Ещё одна ответственность } // Лучше: разделение ответственности async function createOrder(userId: string, items: Item[]): Promise OrderResult { const user = await this.userRepository.findById(userId); const total = this.calculateTotal(items); if (total > user.creditLimit) { return { success: false, error: 'CREDIT_LIMIT_EXCEEDED' }; } const order = await this.orderRepository.create({ userId, items, total }); await this.eventBus.publish(new OrderCreatedEvent(order)); return { success: true, order }; } Уровень 3: Читаемость и поддерживаемость Поймёт ли другой разработчик этот код через полгода? Хорошо ли названы переменные и функции? Достаточно ли комментариев там, где логика неочевидна? Уровень 4: Производительность и оптимизация Проверяется только если есть явные проблемы. Преждевременная оптимизация — корень всех зол. Но N+1 запросы в цикле — это не оптимизация, это баг. Уровень 5: Стиль и форматирование Последний приоритет. В идеале — полностью отдан линтерам (ESLint, Prettier, StyleLint). Ревьюер не должен тратить время на пробелы и запятые. Как писать комментарии к ревью: тон, структура, примеры Качество комментариев определяет, является ли ревью обучающим инструментом или источником демотивации. Несколько принципов, которые меняют культуру ревью в команде. Разделяйте обязательное и необязательное Ревьюер должен чётко маркировать, что блокирует мердж, а что — просто предложение. Хороший способ — префиксы: [blocker] — мердж невозможен без исправления [suggestion] — хорошо бы сделать, но не обязательно [nit] — мелочь, на усмотрение автора [question] — прошу объяснить, не обязательно менять [praise] — хорошее решение, хочу отметить Без такой разметки авторы не знают, на что реагировать первым, и либо переделывают всё подряд, либо игнорируют комментарии. Критикуйте код, а не человека Плохо: «Ты всегда так делаешь — это неправильный подход» Хорошо: «Этот паттерн создаёт проблемы при масштабировании, потому что... Предлагаю рассмотреть альтернативу:» Разница кажется незначительной, но она определяет, открыт ли автор к обратной связи или занимает оборонительную позицию. Объясняйте «почему», а не только «что» // Плохой комментарий: // Используй Map вместо объекта // Хороший комментарий: // [suggestion] В этом месте используется объект как словарь для частых операций // поиска. Map работает быстрее для этого паттерна (O(1) vs amortized O(1) // с меньшим константным фактором), плюс явно выражает намерение использовать // структуру как коллекцию ключ-значение, а не как объект с прототипом. // Подробнее: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map Предлагайте альтернативы, не только проблемы Комментарий «это неправильно» без альтернативы — наименее полезная форма обратной связи. Если вы видите проблему и знаете лучший способ — покажите его. Если не знаете — так и напишите: «Здесь что-то кажется неправильным, но я не уверен в лучшем решении — давайте обсудим». Антипаттерны код-ревью, которые разрушают команды «Нитпикинг» вместо содержательного ревью Ревьюер оставляет 15 комментариев о пробелах и именовании переменных, но не замечает, что функция делает синхронный HTTP-запрос в горячем пути рендеринга. Формально ревью сделано, фактически — нет. «LGTM» без реального чтения Особенно характерно для старших разработчиков, у которых «нет времени». Результат: Junior-разработчик получает одобрение без обратной связи и не растёт; в кодовую базу попадают проблемы, которые потом дорого исправлять. Ревью как соревнование Ревьюер воспринимает каждый PR как возможность показать, что он умнее автора. Комментарии написаны в снисходительном тоне. Автор начинает избегать PR или отправлять изменения в обход ревью. Архитектурные дискуссии в комментариях к PR Ревью — не место для проектирования архитектуры. Если изменение требует принципиальной архитектурной дискуссии, её нужно вынести в отдельный документ или встречу. Комментарии к PR — слишком неудобный формат для таких разговоров. Бесконечные циклы правок Ревьюер одобряет правку, потом на следующем круге замечает что-то новое, что должен был заметить сразу. После трёх-четырёх итераций разработчик готов выбросить компьютер в окно. Причина — ревьюер читает PR по кусочкам, а не целиком с первого раза. Правило: попробуй прочитать весь PR до первого комментария. Процесс ревью: организационные аспекты Кто должен делать ревью Классическая ошибка — назначать ревью самому опытному разработчику команды на все PR. Это создаёт узкое место и выгорание. Лучшая практика: Основной ревьюер — человек, наиболее знакомый с затронутым модулем (code ownership) Дополнительный ревьюер — кто-то, кто с этим модулем не работал (свежий взгляд + передача знаний) Для критических изменений — обязательное ревью от Tech Lead SLA на ревью Ревью без SLA — ревью, которое делается «когда есть время». В нашей практике работает следующая схема: Обычный PR: ревью в течение 24 рабочих часов Хотфикс / критический баг: ревью в течение 2 часов Большой PR (более 400 строк): ревью может занять до 48 часов, об этом предупреждаем заранее Нарушение SLA — сигнал для менеджера, а не личная проблема разработчика. Асинхронное vs синхронное ревью Большинство ревью должны быть асинхронными — это экономит время. Но некоторые ситуации требуют синхронного обсуждения: PR затрагивает архитектурные решения После второго круга комментариев без прогресса Junior-разработчик получает сложные правки и нуждается в объяснении Правило: если дискуссия в комментариях к PR превысила 5 сообщений, переходите в голосовой чат. Инструменты и автоматизация: освободите ревью для важного Всё, что можно автоматизировать — должно быть автоматизировано. Ревьюер-человек дорог и утомляем; CI/CD работает бесплатно и без выходных. Обязательный минимум автоматизации # .github/workflows/pr-checks.yml name: PR Quality Gates on: [pull_request] jobs: quality: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - name: Lint run: npm run lint - name: Type check run: npm run typecheck - name: Unit tests run: npm run test:unit - name: Check bundle size uses: andresz1/size-limit-action@v1 with: github_token: ${{ secrets.GITHUB_TOKEN }} - name: Security audit run: npm audit --audit-level=high Что автоматизируется: Форматирование и стиль (Prettier, ESLint) Типизация (TypeScript compiler, mypy) Покрытие тестами (минимальный порог, например 80%) Размер бандла (Size Limit) Уязвимости в зависимостях (npm audit, Snyk) Дублирование кода (jscpd) Если хоть один из этих чеков не прошёл, PR физически не может быть замержен. Ревьюер не тратит время на то, что машина проверит лучше него. Статический анализ как дополнительный ревьюер SonarQube, CodeClimate, или более лёгкие решения как GitHub Code Scanning — дают хороший сигнал о проблемах ещё до того, как живой человек открыл PR. Особенно ценно для поиска потенциальных NullPointerException, неиспользуемых переменных, мёртвого кода. Код-ревью в контексте разных ролей Ревью для Junior-разработчиков Это в первую очередь образовательный инструмент. Несколько принципов: Не исправляйте всё сразу — выбирайте 2–3 самых важных урока на один PR Объясняйте принципы, а не только правила («это нарушает принцип единой ответственности, потому что...») Отмечайте хорошие решения — это важно для мотивации и ориентирования Предлагайте ресурсы для изу