На что нужно обращать внимание, делая ревью кода

Перевод статьи Udayakumar Rayala «Code review checklist».

Я заметил, что каждая команда и все члены этих команд имеют разные списки того, на что они обращают внимание, когда делают ревью кода. В этом посте я перечислю то, на что смотрю я. Это далеко не полный перечень. Если вы думаете, что стоило бы что-то добавить, то напишите об этом в комментариях.

Дизайн

Необходимо проверить, является ли дизайн нового кода правильным для заданного языка программирования и используемой вами платформы.

Что считать правильным дизайном, команда определяет заранее. Когда появляются новые абстракции и начинает применяться новый дизайн, всегда лучше заблаговременно провести обсуждения на эту тему между разработчиками.

Ревью кода может быть способом убедиться, что все реализовывается так, как договорились. Это способствует уменьшению количества кода, который придется переделывать. Исключением являются случаи, когда вы реализуете что-то новое и хотите получить фидбек по этому поводу. При этом убедитесь, что вы написали достаточно, чтобы ваша идея просматривалась.

Итак, в плане проверки дизайна следует:

Проконтролировать соответствие кода вашим соглашениям (coding conventions) и руководству по стилю. Подобные соглашения и четкое следование им способствуют тому, что код пишется последовательно и с применением лучших методов.
Проверить, нет ли дублирования кода. Такое может произойти, например, из-за недостаточной осведомленности разработчика об уже существующих компонентах, которые можно использовать повторно. Или код могли просто скопипастить откуда-то.
Определить, где упущены возможности рефакторинга. Часто, если разработчик опытный и знающий, то ему проще включить некоторые задачи рефакторинга в качестве части истории/PR, вместо того чтобы потом искать блок времени для его выполнения.
Заметить и предупредить разработчика о потенциальных зависимостях или конфликтах с кодом других разработчиков. Например, когда в разработке или на ревью есть множественные фичи, которые затрагивают те же разделы кодовой базы.
Если в коде представлена нестандартная фича или хак, то должны быть аннотации с тегами #TODO или #FIXME и соответствующим комментарием. В качестве комментария могут выступить ссылка на отрывок кода/патч, использованный по какой-то причине, и указание, каким должно быть идеальное решение.Пример: #FIXME Патч для обхода бага # в <фреймворк> <ссылка>. Удалить после апгрейда до исправленной версии.
Обратить внимание, представлены ли в коде новые библиотеки. Нужно проверить, есть ли в них необходимость вообще и использована ли последняя их версия. Возможно, есть ли лучшие альтернативы?

Тесты

Вышеприведенный перечень, касающийся кода, полностью применим и к тестам.
Нужно убедиться, что код покрыт тестами на всех уровнях – модульном, интеграционном и функциональном.
Проверьте, все ли тесты пройдены.
Посмотрите, соответствует ли результат тестов тому, как в документации описан замысел кода.
Также нужно проверить, выдают ли тесты правильные сообщения об ошибках в случае провала. Например:

Влезьте в шкуру QA

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

Прочтите описание истории, задайте вопросы бизнес-аналитику или собственнику продукта. Проверьте, реализованы ли все упомянутые критерии приемки. Мне кажется, что большая часть багов может быть обнаружена как раз в ходе подобной проверки.
Подумайте о сценариях, не описанных в истории. Проверьте, не нарушают ли новые изменения обычные user flows, работает ли все, как предусмотрено.

Кросс-функциональные (нефункциональные) требования

Также стоит сосредоточиться на кросс-функциональных требованиях, таких как производительность, безопасность, аналитика, журналирование, система оповещений и т. п.

Проверьте, может ли код привести к каким-то проблемам с производительностью. Примеры – проблема запросов N+1 или загрузка всей базы данных в память. Подумайте о запуске этого кода в продакшн-среде и постарайтесь предугадать, какого рода проблемы могут возникнуть. Конечно, вы не найдете их все просто посмотрев на код, но, возможно, ваш опыт поможет вам распознать какие-то распространенные ошибки.
Проблемы с безопасностью можно определить заранее. Проверьте, доступны ли данные только авторизованным пользователям. Почитайте об обычных мерах предосторожности в плане безопасности. Узнайте, что можно предпринять, и поделитесь этой информацией с командой.
Если у вас уже встроена какая-то аналитическая система, проверьте, должен ли интегрироваться с ней новый функционал.
Проверьте, достаточно ли ведется логов для отладки приложения.
Посмотрите, добавлены ли оповещения на случай, если что-то пошло не так.

Непрерывная доставка

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

Проверьте, добавлены ли скрипты миграции данных и выполняются ли они должным образом. Запустите их на своей машине, если это необходимо. Не вносите изменения, которые повлекут за собой потерю данных. Для обнаружения и устранения потенциальных проблем нужно, чтобы фичи, которые идут в продкшн, были автоматически развернуты в промежуточной среде, в которой есть копия продакшн-данных (обфусцированных).
Обратите внимание, не затрагивает ли добавляемая фича какой-нибудь функционал, который вы не намерены релизить.

Кроме того

Нужно проверить, смерджен ли проверяемый код с последним кодом на master branch.
Убедитесь, что будут добавлены инструкции, которым после интеграции этих изменений должны будут следовать любые другие команды. Например, если есть какие-либо серьезные изменения, нужно проинформировать команду QA, в каких зонах запускать регрессию. А если есть какие-либо изменения в контракте API, то нужно поставить в известность потребительские системы. Сюда же относится обновление README для помощи другим разработчикам.

В целом, старайтесь просматривать небольшие изменения и в скором времени делать пул-реквесты, чтобы пораньше получить обратную связь.

[customscript]techrocks_custom_after_post_html[/customscript]
[customscript]techrocks_custom_script[/customscript]