Как проходит Code Review на текущем месте работы

А, ну Code Review, святое дело! Без этого вообще никуда, иначе потом разгребать такое, что ебать мои старые костыли. У нас в Node.js-команде это железное правило: без ревью — ни один коммит в main не проскочит. Сейчас расскажу, как у нас этот цирк с конями устроен.

Вот смотри, начинается всё с того, что разработчик создаёт Pull Request в GitHub. У нас там шаблон заведён, чтобы не присылали просто ссылку на задачу с текстом «всё готово, ёпта». В шаблоне надо описать, что поменялось, дать ссылку на задачу в Jira, и обязательно пройтись по чеклисту: тесты написал? Документацию обновил? Если интерфейсы трогал — скриншоты приложил? Без этого даже смотреть не буду, сразу отправлю на доработку, идиоты.

Дальше система сама назначает одного-двух ревьюверов из тех, кто в теме этого модуля. И вот тут начинается самое интересное. Мы проверяем не просто так, «а работает ли». Нет, мы копаем глубоко.

Первое — кодстайл. У нас стоит ESLint с конфигом @typescript-eslint и Prettier. Если код отформатирован криво или там запятая лишняя — Prettier сам всё поправит при коммите, но если человек зачем-то отключил автоформатирование и наворотил — это сразу подозрение, ёпта. Волнение ебать, что там ещё понаписано.

Второе — архитектура. Это самое важное. Смотрим, чтобы код не превращался в спагетти, где всё зависит от всего. Соответствует ли он нашим принципам чистой архитектуры? Не накрутил ли человек лишних зависимостей, которые потом всю сборку раздуют? Если вижу какой-нибудь костыль, который «пока так сработает» — сразу в комментах спрашиваю: «Мужик, а что будет, когда это масштабировать придётся? Ты веришь, чувак, что это переживёт ещё одну фичу?».

Третье — тесты. Новую логику без тестов мы просто не принимаем. Точка. Используем Jest и Supertest. Если человек написал сложную бизнес-логику, но покрыл её только одним тестом «на счастливый путь» — это пиздец. Говорю: «А давай ещё кейсы, где оно падает? Где невалидные данные? Где сеть легла?». Без этого доверия ебать ноль.

Четвёртое — безопасность и производительность. Нет ли тут потенциальных инъекций? Не забыл ли экранировать данные? Не сделал ли N+1 запрос в цикле, который потом всю базу положит? Это прям must check.

Пятое — читаемость. Названия функций должны говорить сами за себя. Если вижу функцию process() — сразу вопрос: «А чё она обрабатывает-то, блядь?». Сложную логику обязательно просим задокументировать через JSDoc, чтобы через полгода не гадать, что этот кусок кода делает и почему он вообще работает.

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

// Было (до ревью)
async function getData(id) {
  const data = await db.query('SELECT * FROM users WHERE id = ?', [id]);
  return data;
}

Ну я смотрю на это и думаю: «Ни хуя себе, чувак. А если пользователя нет? Вернёшь undefined и потом вся логика посыпется? Прямой запрос к базе из бизнес-логики? Да ты, братан, смелый». И после нескольких раундов обсуждения получается вот такая красота:

// Стало (после ревью)
import { UserRepository } from '../repositories/user.repository';
import { NotFoundError } from '../errors/not-found.error';

/**
 * Получает данные пользователя по ID.
 * @param {string} userId - UUID пользователя.
 * @throws {NotFoundError} Если пользователь не найден.
 */
async function getUserById(userId) {
  const user = await UserRepository.findById(userId);
  if (!user) {
    throw new NotFoundError(`User with id ${userId} not found`);
  }
  return user;
}

Вот видишь разницу? Логика ушла в репозиторий, появилась явная обработка ошибки, всё задокументировано. Красота!

У нас правило — «двух апрувов». Пока два человека не поставили approve, мерджить нельзя. И мы стараемся не тянуть: отвечаем на PR в течение рабочего дня. Комментарии всегда стараемся делать конструктивными, не просто «фигня», а с объяснением, почему так лучше, и со ссылками на наши же гайдлайны или примеры из кода. Чтобы человеку было понятно, а не просто обидно. А то иначе какой смысл?