Пять правил код-ревью, для стажёров, джунов и мидлов

Роман Олеск

Центр экспертизы разработки приложений

Почти никто не любит ревьюить код, считается, что на это тратится слишком много времени. Я совершенно не согласен с этим и здесь покажу, как можно структурировать и ускорить код-ревью.

И для начала поделюсь «эталонной» статьёй на тему: The Standard of Code Review.

Код-ревью для стажёров

Стажёр — это обычно студент или выпускник, который получает практический опыт работы в определённой области и приходит на стажировку для получения знаний.

Стажёр выполняет задачи, которые ему назначают наставники или руководители, редко делает что-то без помощи ментора.

Читаемость кода

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

Если код написан аккуратно, но не соответствует установленному стилю, помечаем его как «дефект». Разнообразие стилей значительно усложняет поддержку проекта.

Дублирование кода

Думаю, что тут всё ясно из названия. От дублирования в коде надо уходить, всегда можно придумать, как этого избежать.

Имена

Код является своего рода документацией, и чем проще и понятнее его читать, тем лучше его понимание.

Мы внимательно рассматриваем переменные, константы, поля классов, имена свойств и так далее, и при необходимости делаем их более описательными и понятными.

Тесты

Автоматизированные тесты — это тоже код, поэтому надо не забывать проверять и их. Важно оценить качество этих тестов, что и как они тестируют, их читаемость, правильность именования и покрытие кода тестами.

Чем меньше MR, тем лучше

Я рекомендую оставлять в изменениях максимум 20–30 файлов. Больше — становится уже сложно читать. Кроме того, MR должен выполнять только одну задачу, которая часто является частью более крупной.

Есть несколько преимуществ от такого подхода:

• Уменьшается вероятность упустить баги.
• Легче достичь высокого качества кода.
• Чем больше изменений в MR, тем сложнее будет откатить код, если потребуется.
• Интеграция изменений становится проще, поскольку конфликты меньше.

Очень редко возникают ситуации, когда невозможно уменьшить размер MR, разбив его на части. Ревьювер имеет право отклонить MR, если он слишком большой.

Код-ревью для джунов

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

Всегда есть что улучшить

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

Объясняйте кодом

Если вас просят объяснить какой-то момент в коде, подумайте, нельзя ли поправить код так, чтобы он был понятен без объяснений. Потому что если один не понял, то и другие могут не понять.

MR должен быть маленьким, но самодостаточным

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

Все необходимые для понимания MR детали должны быть представлены в самом MR. Если в MR добавляется новый метод API, то там же нужно добавить примеры его использования.

Порядок просмотра MR

Сначала надо просмотреть MR в целом: нужен ли он вообще, в правильном ли месте находится (или лучше вынести в отдельную библиотеку), есть ли глобальные проблемы. Нет смысла смотреть на детали реализации, если код вообще не там и не для того.

О любых серьёзных проблемах нужно сообщить сразу, даже если вы ещё недосмотрели MR.

Скорость проверки MR

Скорость экстремально важна. Если раз в несколько дней давать по одному комменту, то автор MR будет жаловаться, что к нему придираются и не дают работать.

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

Код-ревью для мидлов

Мидл-разработчик — это специалист среднего уровня. Он обладает более глубокими знаниями и навыками, чем джуниор, и может выполнять более сложные задачи.

Он уже может делать многое самостоятельно, но помощь в каких-то моментах всё ещё нужна.

Сколько угодно замечаний

В документации по ревью от Гугла есть классная штука, называется nit.

Мелочи и придирки ревьювер помечает префиксом nit: от английского. nitpick (придирка).

Необязательно исправлять такие замечания, однако автор MR может захотеть внести изменения или, даже если не хочет, учесть некоторые моменты на будущее.

«Поправлю потом»

Если разработчик согласен с тем, что в коде есть проблема, но просит заапрувить MR, обещая, что исправит в другой раз, вероятнее всего, это «потом» никогда не наступит.

Поэтому если MR не какой-то срочный багфикс прода, то нужно настаивать на исправлении или в крайнем случае завести задачу и todo в коде с её номером.

Стандартный подход лучше самописного

Почти всегда стандартные принципы построения ПО (software design), базирующиеся на практиках лучших, чем придуманные в муках велосипеды. Поэтому предпочтение нужно отдавать первым.

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

Конфликт ревьювера и автора MR

Сначала идёт попытка найти компромисс в комментариях к MR. Если это не получается, то надо перейти к личному обсуждению. В крайнем случае — привлекать других членов команды. Главное — MR не должен застревать надолго из-за несогласия двух человек.

При этом важно избегать аргументов вроде «Делай так, потому что я сказал». Такой тон общения неприемлем.

Описание MR

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

Правила

• В заголовке должен быть номер задачи и краткое её описание. 
• Внутри дескрипшена должно быть сказано, что было сделано в MR. 
• По традиции применяется императивный (повелительный) стиль, то есть Delete this, а не Deleting this.
• Важно добавить контекст: краткое описание решаемой проблемы, ссылки на необходимые документы (если есть) и так далее. Даже в маленьком MR что-то такое должно быть.
• Описания вроде «Исправлен баг» считаются неадекватными.

Пример

Заголовок: TEAMGAZ-1234: Получение баланса кошелька цифрового рубля

Описание:

• добавлен эндпоинт для мобильного приложения;
• добавлена доменная модель;
• добавлена валидация входного запроса;
• реализован сервис интеграции с ЦБ;
• написаны юнит- и интеграционные тесты.

Чек-лист: на что обращать внимание при просмотре

• Код хорошо спроектирован.
• Код соответствует стайлгайдам.
• Нет кода, который может понадобиться, а может — и нет.
• Код не слишком сложный.
• Наименования (для всего) выбраны хорошо
• Функциональность хорошо сделана с точки зрения пользователей, кем бы они ни были.
• Есть тесты.
• Тесты хорошо спроектированы.
• Комментарии к коду (если есть) понятны и необходимы, объясняют почему так сделано, а не как это сделано.
• Есть документация.

Бонус: несколько дополнительных правил для всех

• Новые инспекции в продуктовом коде появляться не должны, даже если очень хочется.
• Нужно быть вежливым, не переходить на личности. Обсуждать код, а не кодера.
• Нужно не просто выдавать директивы к исправлениям, а объяснять, почему код нужно исправить.
• Выдавайте таски по-разному. Если задача срочная и замораживает рабочий процесс — объясняйте решение и закрывайте MP. Когда сотрудник может, но не думает — подсказывайте и наводите на решение.
• Если проект поставляется с документацией, проверьте её тоже. Если изменения, которые вносятся в код, включают добавление новой функции, убедитесь, что обновили документацию, а затем сравните.