Чтение диффа на корректность

Что это такое

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

Почему это важно

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

Ключевые идеи

Поймите замысел раньше кода

Прочитайте описание, тикет и сообщение коммита первыми и сформулируйте одно своё предложение: что должно делать это изменение? Всё дальнейшее — сравнение с этим предложением. Без него вы судите код относительно своего представления о нём, и так начинаются споры о стиле.

замысел: "отклонять повторные адреса при регистрации"
затем:   проверка идёт до вставки и выдерживает два запроса сразу?

Прочтите всё изменение целиком, прежде чем комментировать

Дочитайте до конца и только потом комментируйте. Находка, поднятая на строке 10, часто закрывается строкой 90, а ревью, комментирующее обрывки, читается как куча несвязанных возражений. Единый связный проход позволяет ещё увидеть форму изменения: это одна идея или три, которые должны были быть тремя диффами?

Ищите классы изменений, где живут дефекты

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

- граница: пусто, ноль, один, максимум, отрицательное, unicode, очень длинное
- путь ошибки: может ли это упасть, и что тогда
- данные: что записывается, в каком порядке, восстановимо ли это
- конкурентность: что будет, если два таких выполняются одновременно
- безопасность: кому это разрешено, и проверено ли это здесь

Считайте тесты частью изменения

Тесты, пришедшие с изменением, показывают, что, по мнению автора, это изменение делает; часто они показательнее продакшн-кода. Спросите, упал бы тест при откате изменения, покрыты ли граничные случаи, не утверждает ли какой-нибудь тест деталь реализации, которая сломается на следующем рефакторинге. Изменение без тестов — вопрос, который стоит поднять, а не автоматический блокер.

Спрашивайте, а не утверждайте

«Кажется, здесь теряется причина ошибки — так задумано?» — на это отвечают; «здесь глотаются исключения» — на это защищаются. Вопросы ещё признают, что вам может не хватать контекста, и это удерживает разговор на коде. Когда вы уверены, говорите прямо и объясняйте последствие — уверенность полезна, когда она конкретна.

Практика

Составьте чек-лист ревью

Возьмите изменение, которое ревьюили недавно, и пройдите по нему заново с записанным чек-листом. Затем примените этот чек-лист к следующему диффу.

[ ] я могу сформулировать замысел одним предложением
[ ] дифф делает одну вещь
[ ] граничные входы: пусто, один, максимум, отрицательное, неожиданный тип
[ ] каждый путь ошибки либо обрабатывает, либо пробрасывает с контекстом
[ ] ни один сбой не проглочен молча
[ ] записи упорядочены и восстановимы, а чтения-записи не гоночны
[ ] права проверены там, где происходит операция
[ ] публичное поведение изменено намеренно, вызывающие обновлены
[ ] тесты падают, если изменение откатить
[ ] у граничных случаев из чек-листа выше есть тест

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

Проследите одно изменение от начала до конца

Возьмите недавно влитое изменение и проследите его поведение через систему: вызывающий, хранилище, путь отказа. Не читайте на стиль вовсе.

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

Частые ошибки

• Ревью на стиль, пока дефекты прячутся ниже — внимание тратится там, где автоматика сильнее; исправить: сначала читайте на корректность, формат оставьте линтеру.
• Комментирование до конца диффа — половина возражений уже снята ниже; исправить: один полный проход, затем один набор комментариев.
• Одобрение потому, что тесты проходят — тесты покрывают лишь то, о чём подумал автор; исправить: проверяйте рискованные классы изменений напрямую.
• Десять находок как равные — предпочтение в имени читается так же серьёзно, как баг с потерей данных; исправить: скажите, что блокер, а что необязательно.
• Уверенность в том, что делает код, без чтения вызывающего — контекст живёт за пределами диффа; исправить: откройте файл, откуда изменение вызывается, прежде чем делать вывод.

Что читать дальше

• Чистый код (01-cs-fundamentals/08-clean-code) — планка читаемости, по которой судят дифф, поэтому прочтите его первым.
• Ревью на ясность и тон (01-cs-fundamentals/17-code-review/02-reviewing-for-clarity-and-tone) — как сказать о найденном так, чтобы это исправили.
• Отладка (01-cs-fundamentals/11-debugging) — системный метод, который хорошее ревью применяет к коду, который ещё не запускался.