Код-ревью на собеседовании: как разбирать чужой код

Вместо задачи на алгоритмы вам показывают сорок строк чужого кода и говорят: «Представьте, что это пул-реквест от коллеги. Что скажете?» Формат выглядит лёгким: писать ничего не надо. Но на третьей минуте вы ловите себя на том, что обсуждаете название переменной, а интервьюер ждёт, когда вы заметите SQL-инъекцию в самом начале функции.

Ревью проверяют и вопросом, и практикой. Вопрос «На что ты обращаешь внимание, когда проводишь код-ревью?» в банке Sobes задавали в 17 компаниях. К практическому варианту нужно готовиться иначе, чем к лайвкодингу: тут оценивают, что вы заметили, в каком порядке и как об этом сказали.

Что оценивает интервьюер

Видите ли вы серьёзные проблемы или только стиль.
Умеете ли отличить «сломает прод» от «я бы написал иначе».
Как формулируете замечания: с вами будут работать люди, и ваши комментарии им читать каждый день.
Задаёте ли вопросы о контексте, прежде чем критиковать.

В каком порядке смотреть

Сначала потратьте минуту на чтение без комментариев и скажите вслух, что код делает. Если это непонятно, спросите: «Это обработчик HTTP-запроса или внутренняя функция? Кто её вызывает?» От ответа зависит, какие проблемы считать серьёзными.

Дальше идите по списку сверху вниз:

Корректность. Делает ли код то, что заявлено, на обычных данных.
Граничные случаи. Пустой ввод, null, ноль, повторный вызов, два одновременных запроса.
Безопасность. Пользовательский ввод в запросах, права доступа, секреты в коде и логах.
Читаемость. Имена, размер функции, сколько вещей она делает сразу.
Производительность. Запросы в цикле, лишние копии, работа, которую можно не делать.
Тесты. Есть ли они и что именно стоило бы покрыть.

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

Пример

Функция применяет промокод к корзине:

async function applyPromo(userId: string, code: string) {
  const promo = await db.queryOne(
    `SELECT * FROM promos WHERE code = '${code}'`
  );
  const user = await db.users.findById(userId);
  if (promo.expiresAt < new Date()) {
    return { ok: false };
  }
  const discount = user.cart.total * promo.percent / 100;
  user.cart.total = user.cart.total - discount;
  promo.usedCount++;
  await db.users.save(user);
  await db.promos.save(promo);
  return { ok: true, total: user.cart.total };
}

Вот как может звучать разбор:

Сначала проверю, что понял правильно: функция находит промокод, проверяет срок, уменьшает сумму корзины и увеличивает счётчик использований. Самое серьёзное здесь в первом запросе: код промокода подставляется в SQL строкой, это инъекция. Нужен параметризованный запрос, и это я бы назвал блокером.

Второе: если промокода с таким кодом нет, promo будет null, и на проверке срока функция упадёт. Нужна явная ветка «промокод не найден» с понятной ошибкой.

Третье: промокод можно применить к одной корзине сколько угодно раз, каждый вызов снова уменьшает total. Я бы хранил применённый промокод отдельно и считал итог от исходной суммы.

Четвёртое: два сохранения идут без транзакции. Если второе упадёт, скидка применится, а счётчик не увеличится. И при двух одновременных запросах usedCount++ потеряет одно обновление. Тут нужна транзакция и атомарный инкремент в базе.

После этого можно перейти к менее срочному: деньги считаются дробными числами, и на скидке 15% появятся копейки с хвостом; лимит использований не проверяется, хотя счётчик есть; в ответе { ok: false } нет причины отказа. И в конце про тесты: просроченный промокод, несуществующий, повторное применение.

Про имена переменных в этом разборе нет ни слова, и это правильно: на фоне инъекции и гонки они не стоят времени.

Как формулировать замечания

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

Называйте приоритет вслух. Трёх уровней достаточно:

Блокер: с этим нельзя мёржить (инъекция, потеря данных, падение).
Стоит исправить: работает, но сломается в предсказуемом случае.
На усмотрение автора: стиль, названия, альтернативная запись.

Там, где не уверены, спрашивайте, а не утверждайте:

Здесь нет проверки лимита использований. Это сознательно, потому что он проверяется выше по стеку, или упущение?

Вопрос честнее утверждения, когда вы не видите остальной код, и на реальном ревью он воспринимается гораздо спокойнее. Интервьюер это отмечает отдельно.

Если в коде что-то сделано хорошо, скажите одной фразой. Ревью, в котором двадцать претензий и ни одного «это понятно написано», в команде читать тяжело.

Если язык незнакомый

Скажите об этом сразу и предложите, как будете работать:

На Kotlin я не писал, синтаксис читаю, но идиомы могу не знать. Давайте я разберу логику и работу с данными, а про особенности языка буду спрашивать.

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

Типичные ошибки

Начинать со стиля и не дойти до багов.
Выдать список из пятнадцати пунктов без приоритетов.
Переписывать код вместо ревью. Просили оценить, а не показать свою версию.
Критиковать автора: «кто так пишет». Замечание всегда о коде.
Молча читать пять минут. Думать вслух здесь так же важно, как на лайвкодинге.

Навык пригодится и с другой стороны: на защите тестового задания ревьюить будут уже ваш код.

Как тренироваться

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

Второй способ: откройте свой пул-реквест полугодовой давности и разберите его вслух по списку, с приоритетами. Чужим этот код уже успел стать.