На Temlead Conf Владимир Романько из Лаборатории Касперского рассказывал про Code Review. Я попробовал записать конспект по ходу доклада. Вот что получилось.

На code review жалуются:

1. Не лучший способ для поиска алгоритмических багов.
2. Бесполезно потому как выявляются только мелочи.
3. Тормозит и удорожает разработку.
4. Демотивирует. Это инструмент для самоутверждения ревьювера, который докапывается.
5. Плохой инструмент для обмена знаниями.

Неужели всё плохо? Нет! Это мифы в том смысле, что пункты выше не обязаны быть частью ревью и от них можно избавиться.

Не лучший способ для поиска алгоритмических багов

Баги должны обнаруживаться с помощью тестов. Но есть классы алгоритмов которые можно обнаружить только на Code Review:

1. Уязвимости. Конечно, есть множество инструментов: сканеры, fuzzing-сканеры и статический анализ кода. Но они обнаруживают только если повезёт или работают с сильно специфическими уязвимостями. Ручные тестировщики редко обнаруживают такие баги. Pentest слишком дорог и долог. К тому же, пентестеры начинают всегда с того же code review.
2. Утечки ресурсов. Автоматизированные тесты не обнаруживают такие ошибки. Ручные тестировщики тоже. Можно гонять нагрузочные тесты, но это долго.
3. Производительность. Похоже на утечки ресурсов. Медленно и дорого. Как следствие — плохое покрытие. Пример — сброс кеша для всех данных вместо сброса для конкретного ресурса. Найти такое сложно. Найдётся, скорее всего, уже в проде.
4. Race conditions. Опять же, или review или прод. На проде очень и очень сложно понять, в чём дело. Не воспроизводится.
5. Все другие ошибки в сложных и трудно-воспроизводимых сценариях.

Бесполезно потому как выявляются только мелочи

1. Часто проблемы при Code review отражают глубинные проблемы. Если не находится почти ничего — bus factor = 1. Решение — шарить экспертизу.
2. Программисты не умеют декомпозировать задачи. Жирные куски тяжко ревьюить. Решение — бить на мелкие части.

Тормозит и удорожает разработку

1. Ревью превращаются в чаты с гигантскими задержками. Решение — два варианта действий:
   1. Устно договориться, фиксировать договорённости.
   2. Молча поправить.
2. В pull request вообще не те решения архитектурно. Решение: дизайн-ревью (сначала обдумать и обсудить) + короткие циклы обратной связи. Если и потеряем время, то не слишком много.
3. Обязательные ревьюверы — бутылочное горлышко. Решение: peer review. Сами разработчики инспектируют друг друга, а лид модерирует:
   1. Следит чтобы была внятная аннотация (как и почему решил задачу). Это помогает быстрее разобраться.
   2. Следит за токсичностью. Не допускает.
   3. Добавляет ревьюверов.
   4. В крайнем случае может сам заводить баги.

Демотивирует

1. Сводить к минимуму общение в виде текста.
2. Не допускать комментов с наездами, не использовать повелительное наклонение.
3. Если очень плохой код — не заваливать дефектами. Код нужно сделать достаточно пригодным для master. Слишком много комментов триггерят защитное поведение.
4. Не комментим стилистические дефекты.
5. Заводим баги на чужой код.
6. Критикуешь — предлагай.
7. Вкусовщину не допускаем. Большинство программистов уверены что они программируют лучше коллег. Решение автора должно всегда побеждать.
8. Холивары нужно откладывать. Через какое-то время проще договориться.
9. Демократия для незначительных вопросов. Выносим мелочи на голосование.

Плохой инструмент для обмена знаниями

Нет. Code review:

1. Переносит культуру написания кода.
2. Помогает распространять информацию об изменениях в коде.
3. Позволяет получить навык применения теоретических знаний в боевых условиях с учётом конкретной команды.

Выводы

Да, проблемы есть, но они решаемы.