Code Review

By Pavel Bragin 2,968 September 19, 2016

Category:

Code Review проводится в назначенных парах не мене 2 месяцев с даты формирования пары для лучшего понимания проекта поверяющими сторонами.

Таблица результатов ревью здесь.

Советы по Code Review:

Для всеx:

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

Для ревьюверов:

Ревью кода должно быть частым и короткими сессиями. Эффективность ваших ревью снижается примерно через час. Так что откладывание ревью, чтобы просмотреть их все в одной громадной сессии не поможет никому. Найдите время в течение дня, совпадающее с вашими перерывами — чтобы не нарушать состояние потока и сформировать привычку. Ваши коллеги будут благодарны вам за это. Ожидание может быть неприятно, и они могут решить вопросы быстрее, пока код еще свеж в их головах.
Сказать «все хорошо» — это нормально. Не будьте придирчивы, вам не нужно искать проблему в каждом ревью.
Используйте чеклист. Чеклисты для ревью кода обеспечивают согласованность — они дают убедиться, что каждый отслеживает важные и распространенные ошибки.

Для авторов кода:

Код должен быть кратким. Через 200 строк кода эффективность кода значительно снижается. К тому времени, когда вы просмотрите 400 строк, они становятся почти бессмысленными.
Обеспечьте контекст. Давайте ссылки на любые связанные тикеты или спецификации. Есть инструменты для ревью кода типа Kiln, которые помогут с этим. Давайте короткие, но полезные сообщения к коммитам и много комментариев в коде. Это поможет ревьюверу и вы получите меньше вопросов.

Обратите внимание на чек лист для code review, на данный момент это довольно общий список с которого мы начнем. В процессе работы мы будем его трасформировать и улучшать. Предложения по улучшению принимаются в письменном виде на почту и в общем чате. Чек лист нужно оптимизировать его под специфичные сценарии и проблемы. Отличный способ сделать это — отмечать вопросы, которые возникают в команде во время ревью кода в течение короткого времени. С этой информацией мы сможем найти самые распространенные ошибки, чтобы затем внести их в ваш чеклист.

Не бойтесь выбрасывать из чеклиста те элементы, которые нам не подходят (вы можете захотеть оставить редко встречающиеся, но все же критические пункты — например, связанные с безопасностью) Пишите о них немедля!

Начните использовать и держите в актуальном состоянии

Чек-лист CodeReview

Общее

Работает ли код? Выполняет ли он свои прямые обязанности, корректна ли логика, и т. д.
Легок ли код для понимания?
Соответствует ли код вашему стилю написания кода? Обычно это относится к расположению скобок, названиям переменных и функций, длинам строк, отступам, форматированию и комментариям.
Есть ли в ревью избыточный или повторяющийся код?
Является ли код независимым, насколько это возможно?
Можно ли избавиться от глобальных переменных или переместить их?
Есть ли закомментированный код?
У циклов есть установленная длина и корректные условия завершения?
Может ли что-то в коде быть заменено библиотечными функциями?
Может ли быть удалена часть кода, предназначенного для логгирования или отладки?

Безопасность

Все ли входные данные проверяются (на корректный тип, длину, формат, диапазон) и кодируются?
Обрабатываются ли ошибки при использовании сторонних утилит?
Выходные данные проверяются и кодируются (прим. пер.: например, от XSS)?
Обрабатываются ли неверные значения параметров?

Документация

Есть ли комментарии? Раскрывают ли они смысл кода?
Все ли функции прокомментированы?
Есть ли какое-то необычное поведение или описание пограничных случаев?
Использование и функционирование сторонних библиотек документировано?
Все ли структуры данных и единицы измерения описаны?
Есть ли незавершенный код? Если есть, должен ли он быть удален или помечен маркером типа «TODO»?

Тестирование

Является ли код тестируемым? Например, он не должен содержать слишком много зависимостей или скрывать их, тестовые фреймворки должны иметь возможность использовать методы кода, и т. д.
Есть ли тесты и если есть, то достаточны ли они? Например, они покрывают код в нужной мере.
Юнит-тесты на самом деле проверяют, что код предоставляет требуемую функциональность?
Все ли массивы проверяются на «выход за границы»?
Может ли любой тестирующий код быть заменен с использованием существующего API?
Успехов!