В нашей компании существуют правила ревью, и в целом, инфографику выше можно считать верной, но мы пошли глубже и поговорили с Женей — главным инженером-программистом. Узнали про основную цель и принципы процесса ревью, а также составили целый чек-лист для проверки кода!
Основная цель код-ревью заключается в том, чтобы гарантировать постоянное улучшение кодовой базы проекта. (Авторство не наше, взято со статьи на хабре)
Жень, какие принципы есть у ревью?
На мой взгляд, правила и принципы должен диктовать сам проект: его сложность, архитектура, количество разработчиков в команде.
Ключевым моментом является то, что не бывает «идеального» кода — бывает только код получше. Проводя ревью, вы не должны требовать от автора полировать каждый крошечный фрагментик. Здесь важен баланс между необходимостью дальнейших улучшений и важностью предлагаемых изменений. Вместо того, чтобы стремиться к идеалу, рецензент должен стремиться к непрерывному улучшению. Коммит, который в целом улучшает ремонтопригодность, читаемость и понятность системы — нельзя задерживать на дни или недели, потому что он не «идеален».
Ревью не должно занимать более одного часа на мердж реквест, иначе оно превращается в пытку.
Женя составил чек-лист того, что можно успеть за час:
Функциональность
Делает ли мердж реквест то, что задумал разработчик? Хорош ли код для пользователей нашей системы? Под «пользователями» подразумеваются и конечные пользователи (если их затрагивает изменение), и разработчики (которым придётся «использовать» этот код в будущем).
В основном, мы ожидаем, что ещё до коммита разработчики протестируют свой код достаточно хорошо, чтобы он правильно работал. Но как рецензент, вы всё равно должны думать о крайних случаях, искать проблемы, пытаться думать как пользователь и даже при чтении кода смотреть на отсутствие очевидных ошибок.
Найдите файл или файлы, которые представляют «основную» часть этого мердж реквеста или реализуют основную бизнес-мысль (то есть ту, без которой весь остальной код не нужен ни программистам, ни пользователям системы). Часто существует всего одна ОСНОВНАЯ бизнес-функция, для которой и создавался этот мердж реквест. Сначала посмотрите на эти основные части. Это помогает понять контекст меньших частей кода и, как правило, ускоряет выполнение код-ревью.
Если видите серьёзные проблемы в этой основной части мердж реквеста, вы должны немедленно отправить эти комментарии, даже если у вас нет времени, чтобы просмотреть остальную часть мердж реквеста прямо сейчас. Фактически, просмотр остальной части может оказаться пустой тратой времени, потому что если проблемы достаточно значительны, многие другие рассматриваемые фрагменты кода исчезнут и не будут иметь значения.
Тесты
Убедитесь, что тесты правильны, разумны и полезны. Тесты не проверяют сами себя, и мы редко (никогда) пишем тесты для наших тестов — человек должен сам убедиться, что тесты валидны.
Именование
Разработчик везде выбрал хорошие имена? Хорошее имя достаточно длинное, чтобы полностью передать то, чем является или что делает элемент, не будучи настолько длинным, что становится трудно читать.
Комментарии
Написал ли разработчик чёткие комментарии на понятном языке? Действительно ли все комментарии необходимы?
Резюме
При выполнении код-ревью следует убедиться, что:
- Код хорошо спроектирован;
- Хорошая функциональность для пользователей кода;
- Код не более сложен, чем должен быть;
- Разработчик не реализует то, что может понадобиться в будущем с неизвестными перспективами;
- Код обложен соответствующими модульными тестами;
- Тесты хорошо разработаны;
- Разработчик везде использовал чёткие имена;
- Комментарии понятны и полезны, и в основном, отвечают на вопрос «Почему?», а не «Что?»;
- Код надлежащим образом документирован;
- Код соответствует нашим руководствам по стилю.
И помните, у вас всего час на ревью!
Если вы понимаете, что не успеваете посмотреть код полностью — нужно идти по следующим приоритетам:
- Ревьювим «основную» часть;
- Ревьювим тесты;
- Ревьювим остальной код.
А если кода много и он важен, можно привлечь нескольких ревьюверов, попросить их посмотреть свою часть из списка выше.