Помню, в одном стартапе мы попробовали отменить code review. "Сэкономим время на разработку," — подумали. Через месяц половина багов в production приходила из pull request'ов, которые раньше бы поймали. Пришлось вернуться. С тех пор я вижу ревью кода не как заформальность, а как единственный способ сохранить здоровье кодовой базы.
Статья для тех, кто только внедряет ревью, и для тех, у кого уже есть процесс, но он буксует. Разберу, как это работает на самом деле.
Зачем вообще нужен code review
Короче, code review — это не про придирчивого senior'а, который ищет косяки. Это про четыре вещи:
Баги ловятся до production. Да, тесты ловят часть. Но логические ошибки, граничные случаи, race conditions — это часто видно только в контексте всего кода. На одном проекте мы внедрили обязательный ревью, и количество багов в production упало на 40% в первый месяц.
Знание кодовой базы распределяется. Если только один человек знает, как работает core модуль, команда зависит от него. Ревью заставляет других разработчиков смотреть в разные части системы. Это спасает, когда кто-то уходит или болеет.
Стандарты соблюдаются. Без ревью у каждого свой стиль кода, своя логика обработки ошибок, свои имена переменных. Через месяц это становится кошмаром. С ревью — хоть какой-то консенсус.
Junior'ы учатся. Честно? Это лучше, чем любой курс. Когда senior оставляет комментарий "здесь ты создаёшь утечку памяти, потому что...", junior запоминает на годы.
Как организовать процесс ревью
Я видел команды, где ревью занимает неделю, потому что никто не знает, кому одобрять. Видел команды, где ревью — это "LGTM" от первого встречного. Оба подхода — в дно.
Вот что работает:
Сначала опреди, кто ревьюит. Обычно это 1-2 человека на PR. Если team маленькая (2-5 человек), подойдёт round-robin — по очереди смотрит следующий, кто свободен. Если team побольше (10+), можно разделить по модулям: для изменений в API ревьюит архитектор, для фронта — фронтенд-лид.
На Gitlab/GitHub можно настроить CODEOWNERS файл:
# .gitlab/CODEOWNERS или .github/CODEOWNERS
/src/api/ @ivan @maria
/src/frontend/ @petr
/tests/ @ivan
/docs/ @anyone
Теперь PR автоматически просит ревью у нужных людей.
Сроки. "Посмотрю на выходных" — это не сроки. По-хорошему, ревью должно быть в течение часа-двух. Если PR висит 3 дня, разработчик уже забыл, что там писал, и переключился на новую задачу. Когда потом приходят замечания, всё нужно перечитывать.
На одном проекте завели правило: если ревьюер не ответил за 2 часа, может одобрить second reviewer или даже сам автор (если не критичный код). Ускорило процесс в разы.
Общение. Ревью — это не "я нашёл баг и ты неправ". Это диалог. Если ревьюер не уверен, почему автор выбрал такой подход, нужно спросить, а не критиковать.
Я видел comment вроде: "Это неправильно. Нужно переписать." Вместо: "Здесь возможен race condition. Может, использовать mutex вот так? [код]".
Второй вариант — и понятнее, и respectful.
Типичные ошибки при ревью кода
Ревьюит уставший человек. Читать чужой код — это не скроллинг ленты. Нужна концентрация. Если ревьюер уже 8 часов в кодере, he won't catch anything. На одном проекте завели правило: code review только до 4 вечера, когда ещё свежая голова. Звучит странно, но сработало.
Ревьюит тот, кто не знает контекст. "Почему ты добавил 500 строк кода в один файл?" А потому что задача была такая, и это обсуждалось на стендапе неделю назад. Перед ревью автор должен написать описание: что нужно было сделать, почему выбран именно такой подход, есть ли alternative solutions.
На GitHub/GitLab это pull request description. Заполни нормально — сэкономишь часы на вопросы.
Нет чеклиста. Если каждый ревьюер проверяет что-то своё, половина проблем проскочит. Нужен общий список того, что смотрим всегда.
Ревью становится личным. "Я бы написал по-другому" — это не причина просить переделать код. Если код работает, безопасен и понятен — одобряй. Personal preferences in style — это для linter'а, не для человека.
Чеклист для code review
Вот что я всегда проверяю и советую проверять:
Логика и корректность. Работает ли код так, как задумано? Нет ли off-by-one ошибок, null pointer exception'ов, утечек памяти? Если это асинхронный код, нет ли race conditions?
# Плохо
def get_user(user_id):
user = db.query(f"SELECT * FROM users WHERE id = {user_id}")
return user
# Хорошо
def get_user(user_id):
user = db.query("SELECT * FROM users WHERE id = ?", (user_id,))
return user
Первый вариант — SQL injection. Второй безопасен.
Производительность. Нет ли N+1 queries? Нет ли O(n²) алгоритмов там, где можно O(n log n)? Нет ли утечек памяти в long-running процессах?
Помню, junior написал:
for user in users:
for order in db.query(f"SELECT * FROM orders WHERE user_id = {user.id}"):
print(order)
Если users 1000, это 1000 queries. С join'ом — одна query. Огромная разница.
Безопасность. SQL injection (уже выше), XSS, CSRF, hardcoded secrets, неправильная аутентификация. Если код работает с user data, нет ли утечек?
Читаемость. Понимает ли другой разработчик, что здесь происходит, без комментариев? Имена переменных понятные? Функция не делает слишком много?
# Непонятно
def proc(d):
r = []
for i in d:
if i > 5:
r.append(i * 2)
return r
# Ясно
def filter_and_double(numbers):
return [num * 2 for num in numbers if num > 5]
Тесты. Если добавлен новый функционал, есть ли тесты? Если изменена логика, обновлены ли тесты? Edge cases покрыты?
Документация. Если изменился public API, обновлена ли документация? Если добавлена сложная функция, нужен ли комментарий с объяснением?
Стиль и форматирование. Соответствует ли код style guide'у команды? Нет ли случайно закомиченного debug кода?
Но! Не трать на это 30% времени ревью. Для стиля нужен автоматический linter, не человек.
Автоматизация: спаси себя от рутины
Вот честно: если ты проверяешь руками, что переменная названа с camelCase вместо snake_case, ты тратишь время впустую.
Linter и formatter. Настрой ESLint для JavaScript, pylint для Python, golangci-lint для Go. Запусти их в CI/CD pipeline. Если код не проходит — PR не merge'ится. Точка.
# .gitlab-ci.yml
lint:
stage: test
script:
- pylint src/
- black --check src/
only:
- merge_requests
Type checking. Если используешь Python — mypy. Если TypeScript — встроенный type checker. Половина багов ловятся на этом этапе.
Автоматические тесты. Unit tests, integration tests, coverage. Если coverage упал — PR не merge'ится. Это спасает.
Security scanning. Есть инструменты вроде Semgrep, Bandit, OWASP Dependency-Check, которые ловят типичные уязвимости автоматически.
И вот здесь важный момент: после того как ты автоматизировал все эти проверки, human review становится про то, что машина не видит. Про дизайн решения, про производительность, про то, имеет ли это смысл для бизнеса.
Это гораздо ценнее, чем искать forgotten debug print.
Практические советы из реальных команд
Держи PR маленькими. Если PR на 1500 строк, никто не будет нормально ревьюить. Большие PR тянут на часы ревью, в них легче пропустить баги. Ориентируйся на 200-400 строк изменений на PR.
Ревью в паре. Иногда полезно сесть вместе с автором и пройтись по коду. Вживую можно обсудить архитектурные решения быстрее, чем в комментариях.
Rotating reviewers. Не давай ревью всегда одному и тому же человеку. Все устают, все обгорают. Ротируй.
Не требуй идеального кода. "Лучше готово, чем идеально" — это не про качество, это про баланс. Если код работает, безопасен и понятен, одобри. Рефакторинг можно сделать отдельным PR'ом.
Документируй решения. Если в ревью было жаркое обсуждение, почему выбран именно этот подход, напиши в ADR (Architecture Decision Record) или в комментарии кода. Через год никто не будет помнить, почему так.
Когда человеческого ревью недостаточно
Если честно, в 2024 году полагаться только на human review наивно. Люди устают, отвлекаются, пропускают ошибки.
Здесь на помощь приходит автоматизация на AI. Я имею в виду не просто linter'ы, а инструменты, которые анализируют код как человек — понимают логику, видят потенциальные проблемы, ловят security issues.
Мы в Distiq как раз этим и занимаемся. Это AI-бот, который подключается к GitLab, GitHub или GitVerse и автоматически ревьюит каждый merge request. Бот оставляет inline-комментарии с замечаниями, находит баги, уязвимости, проблемы с производительностью.
Главное — это не замена человеческому ревью, а помощник. Бот берёт на себя рутину (стиль, типичные ошибки, security patterns), а человек ревьюит архитектуру и логику.
Интегрируется за пару минут, работает с Python, JavaScript, TypeScript, Java, Go и другими языками. Данные остаются на серверах в РФ.
Но главное всё равно — организованный процесс, хорошая культура ревью в команде и понимание, что code review спасает от проблем в production.