iTechArt logo

Code review по всем правилам: чек-лист от разработчиков iTechArt

Development & QA

Что будет, если случайно или намеренно забыть про code review, можно ли перепоручить кому-то данную фазу работы и есть ли универсальный эффективный алгоритм для проверки кода? На эти вопросы точно знают ответ Илья Гумбар и Владислав Наруцкий, Software Engineers iTechArt. 

Наши собеседники
Илья Гумбар.jpg

Илья Гумбар

Software Engineer iTechArt

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

Владислав Наруцкий.jpg

Владислав Наруцкий

Software Engineer iTechArt

Благодаря code review отслеживает, чтобы код передавал намерения разработчика и был понятен даже человеку, который не погружен в данную проблему.

Насколько важен code review? Что будет, если его пропустить?

ИЛЬЯ ГУМБАР: Code review безусловно важен при командной работе. В случае успешно налаженных процессов code review 

  • помогает контролировать технический долг проекта
  • улучшает читаемость и поддерживаемость кода
  • снижает количество дефектов/багов
  • способствует knowledge sharing 

Как следствие, code review может помочь уменьшить издержки на разработку качественного приложения, что должно напрямую соответствовать интересам заказчика. 

Даже в случае emergency правок, code review должен быть обязательным шагом. Игнорирование данной ступени в долгосрочной перспективе ведет к увеличению сроков разработки и, соответственно, к удорожанию.

ВЛАДИСЛАВ НАРУЦКИЙ: Я считаю, что этап code review достаточно важен, и относиться к нему надо ответственно. Если его пропустить, либо вести себя халатно, это может повлечь за собой некоторые временные издержки, абсолютная величина которых зависит от различных факторов (например скорость сборки и развертывания продукта на различных окружениях). Делясь собственным опытом, могу сказать, что мне нередко удается выявить различные виды ошибок – например, ошибки в бизнес-логике, неправильная обработка входных/выходных параметров функций, и т.д. – еще в ходе code- eview. Конечно, эти ошибки так или иначе были бы позже обнаружены QA инженерами, но как я говорил выше – это повлечет за собой временные издержки. Разработчику надо будет снова запустить проект, поправить, потом (возможно) будет ещё одно code review, потом снова сборка и развертывание на QA окружение. А это всё время, которое можно сэкономить при тщательном code review.

Также у code-review есть ещё одна не менее важная цель – это поддержание кода и архитектуры в «чистоте». Я не буду сейчас подробно останавливаться на определении «чистоты», про это написано уже немало книг (например «Чистый код» и «Чистая архитектура» Р. Мартина), но просто скажу одну, возможно очевидную, вещь – на самом деле программисты работают не с машинами, а с людьми посредством своего кода. Поэтому в течение code review я также слежу за тем, чтобы код передавал намерения разработчика, и был понятен даже человеку, который не погружен в контекст, ведь скорее всего в будущем код придется поддерживать другим людям. А если позже в нем будет непросто разобраться, то это снова повлечет за собой временные издержки.

Насколько code review – трудоёмкий этап? Сколько времени на него требуется?

ИЛЬЯ ГУМБАР: Все зависит от проекта и вносимых изменений. Как правило, большинство команд стараются контролировать размер правок за счет соблюдения некоторых правил таких как 

  • PR (pull request) должен содержать изменения которые затрагивают только один use-case.
  • PR не должен содержать одновременно и новую функциональность и рефакторинг и т.д.

Качество проверки обратно пропорционально размеру проверяемых изменений. Т.е. чем больше PR, тем выше шанс что либо упустить. Соответственно, временные затраты на code review контролируются командой и как правило не превышают 30 минут.

ВЛАДИСЛАВ НАРУЦКИЙ: Я бы сказал, что достаточно трудоёмкий, т.к. одной из сложностей является понять, как думает другой человек. Бывает, что ты уже точно знаешь, как бы ты решил этот вопрос, а потом при проверке видишь совсем не то, что ожидал. И тут есть момент, когда важно не поддаться искушению, что любое решение, которое не совпадает с твоим – неправильно. Надо все-таки дать альтернативному мнению шанс и вникнуть в него, возможно оно более оптимальное. Это бывает сделать непросто, т.к. в твоей голове уже сложился образ правильного решения.

Еще одной сложностью, я бы сказал, является сложность решаемой задачи. Когда идёт имплементация какой-либо сложной бизнес-логики, или бизнес-флоу, случается, что затронуты множество файлов в разных частях приложения. Вникнуть в такое решение бывает сложнее, и оно, в свою очередь, потребует больше времени для code review. Следует также обращать внимание на то, были ли затронутые какие-либо переиспользуемые вещи. Если да – то надо оценить, а могло ли данное изменение что-либо где-либо сломать, и т.д.

Если постараться дать конкретную временную оценку - то я бы сказал, что code-review может занимать от 5-10 минут до 1-2 часов в зависимости от сложности и объёма задачи.

На кого можно делегировать code review?

ИЛЬЯ ГУМБАР: В различных командах могут применяться различные стратегии. Качество и количество ревьюверов также может меняться в зависимости от области, куда вносятся изменения. Как пример, изменения одобряют как минимум двое: Tech Lead и разработчик, который так или иначе был вовлечен в затрагиваемую функциональность. 

С точки зрения knowledge sharing, чем больше людей просмотрит PR, тем лучше. Так что в случае, когда PR привносит изменения в ядро проекта, либо когда расширяется библиотека общих компонентов, будет не лишним оповестить команду (либо ее часть).

ВЛАДИСЛАВ НАРУЦКИЙ: Я думаю, что делегировать code review можно любому достаточно опытному коллеге из группы, который хорошо ориентируется в предметной области и бизнес-требованиях продукта, а также в архитектуре проекта/системы. Также данный ревьювер должен уметь анализировать различные подходы и решения, уметь оценить их плюсы и минусы, чтобы по итогу выбрать максимально простое и оптимальное из всех возможных.

Автор кода VS ревьюер. Какие обязанности есть у каждой стороны?

ВЛАДИСЛАВ НАРУЦКИЙ: Я думаю, что обязанности сторон примерно следующие. Автор кода должен найти максимально простое и оптимальное решение, при этом сохранив код в «чистоте» (о которой уже было сказано выше). А ревьювер, в свою очередь, должен это подтвердить. Если же это не так, то ревьюверу следует дать конструктивный фидбек и предложить альтернативы автору. Также на ревьювера ложится обязанность проанализировать, действительно ли все были случаи покрыты. То есть не только убедиться, что код, который был написан корректен, но и также убедиться, что никакой другой код не должен быть написан.

ИЛЬЯ ГУМБАР: Обе стороны должны быть дружелюбны, вежливы и уважительны друг к другу в общении. Также нужно помнить, что цель у двух сторон одна – улучшить какой-либо аспект системы. 

Обязанности автора:

  • Подготовить PR к ревью и убедиться, что он соответствует принятым правилам (например: PR должен быть небольшим, ссылаться на выполняемую задачу, должен содержать описание вносимых изменений и применяемых подходов, должен содержать тесты для вносимых изменений, code style должен соответствовать стандартам и т.д.).
  • Проанализировать и отреагировать на каждый комментарий. Бывают ситуации, когда те или иные замечания не могут быть оперативно исправлены (emergency fix). В этом случае автор обязан создать technical dept на будущее для того, чтобы исправить замечание, когда ситуация будет позволять внести необходимые изменения.
  • Внести требуемые изменения или обосновать, почему то или иное замечание не может быть исправлено.

Обязанности ревьювера:

  • Внимательно проанализировать предлагаемые изменения по следующим критериям: архитектура, функциональность, сложность, тесты, code style, best practices и.д. 
  • На заключительном этапе ревьювер должен запустить изменения у себя на машине и проверить их соответствие поставленной задаче.
  • Оставить список замечаний на основе анализа изменений. Необходимо, чтобы правки реально способствовали улучшению качества кода по какому-либо аспекту. 

В случае наличия налаженных процессов конфликтные ситуации исключены, а в случае их наличия регламентированы способы их устранения.

С помощью каких сервисов можно делать code review? Выдели их основные достоинства и недостатки

ИЛЬЯ ГУМБАР: Как правило, большинство сервисов управления проектами имеют те или иные встроенные возможности для code review. Мне не довелось проводить сравнительный анализ каждой, но могу выделить функциональность, которая так или иначе должна быть реализована для максимального удобства:

  1. Связь с бэклогом 
  2. Связь системой CI/CD
  3. Связь с системой документации /wiki 
  4. Возможность оставлять замечания к строчке, блоку, либо ко всему PR
  5. Возможность создавать задачи на основе замечаний и связывать их с PR
  6. Возможность предлагать изменения (в виде diff)
  7. Возможность отслеживать и контролировать статус правок
  8. Возможность автоматически назначать ревьювера(ов), используя гибкую систему правил
  9. Поддержка расширенного синтаксиса для оформлени PR и замечаний (ссылки, картинки, чек-листы и т.д.)

Примеры: GitHub, Azure DevOps, Atlasian tools

ВЛАДИСЛАВ НАРУЦКИЙ: Наиболее часто приходилось делать code review через BitBucket и GitHub. Поэтому, сравнивая эти два сервиса между собой, могу отметить следующее:

  • BitBucket более удобен тем, что отображает структуру файлов и папок, что упрощает навигацию по сравнению с GitHub.
  • В GitHub же более удобно организовано комментирование, а также подсветка синтаксиса, что упрощает непосредственно сам процесс code review.

Также наслышан, что некоторые Git инструменты (конкретно десктоп клиенты) поддерживают просмотр Pull Request'ов прямо из приложений – например, GitKraken. Думаю, что это более удобно по сравнению с code review в браузере, но пока что лично не приходилось их пробовать.

У Google есть свой стандарт правильного код ревью. Пользуешься ли ты этим стандартом в работе?

ИЛЬЯ ГУМБАР: Наши текущие процессы во многом пересекаются с процессами описанными в Google's Engineering Practices. Используем с поправкой на особенности наших задач. 

ВЛАДИСЛАВ НАРУЦКИЙ: Некоторые из принципов Google присущи моей работе. Например, стараюсь избегать личных предпочтений касательно некоторых вещей и всегда с ребятами обсуждаю детали, которые мне непонятны, чтобы вникнуть в поток размышлений автора кода. Также слежу за соблюдением code styles, и придерживаюсь их сам.

Твой личный алгоритм код ревью?

ВЛАДИСЛАВ НАРУЦКИЙ: Мой личный алгоритм code review состоит примерно из следующих шагов:

  1. Остановиться и подумать/вспомнить, о чём данный Pull Request, какого рода изменения я ожидаю в нём увидеть. Это помогает начать погружаться в новую задачу.
  2. Просмотр изменений в файлах, которые содержат переиспользуемые решения. При этом оценить, могут ли данные изменения проявить себя где-то ещё/что-либо сломать.
  3. Просмотр изменений в файлах, специфичных для данной задачи.
  4. Выкачивание себе данного Pull Request'а, запуск и тестирование основной функциональности, специфичных кейсов.
  5. В зависимости от результатов, полученных в пунктах выше, далее возможны следующие исходы:
    • Если требуются правки, то даю фидбек (либо оставляю комментарии в Pull Request, либо созваниваемся и обсуждаем вместе) и прошу внести изменения. Потом, соответственно, процесс code review повторяется, но пристальное внимание уже уделяется только файлам, в которых были изменения.
    • Если все хорошо либо требуются очень небольшие косметические изменения (которые я сам в состоянии поправить за 1-2 мин), то мержу данный Pull Request и code review завершается.

ИЛЬЯ ГУМБАР: Приведу чек-лист, который использую сам:

Проверка на соответствие стандартам

  • Много ли изменений и способно ли это затруднить проверку?
    • Можно ли их разбить на части? Было бы лучше, если бы это было 2 отдельных PR?
    • Содержат ли изменения как новую функциональность, так и рефакторинг? (Это не всегда хорошее решение)
    • Достаточно ли хорошо сформулированы названия и описания коммитов?
  • Методы/функции и классы разумных размеров?
  • Требуется ли вынесение констант из кода в конфигурацию либо объявление их в отдельном месте для простоты модификации?
  • Есть ли необязательные комментарии либо закомментированный код?
  • Есть ли “мертвый” код (код который никогда не вызывается/используется и не влияет на работу программы)? Для этого скорее всего потребуется проанализировать изменения у себя на машине.
  • Появляются ли новые предупреждения при сборке?

Поддержка

  • Необходимо ли дополнительное логирование/мониторинг? Достаточно ли будет диагностической информации, чтобы проанализировать возможные проблемы после развертывания?
  • Корректно ли обрабатываются возможные ошибки и исключительные ситуации?
  • Легко ли код понять и поддерживать? (KISS, YAGNI) (KISS - Keep It Simple, Stupid YAGNI - You Ain’t Gonna Need It)
  • Являются ли изменения обратно совместимыми с текущим кодом? Изменения в контрактах (API или сообщений) обязаны версионироваться, если они не являются обратно совместимыми.
  • Есть ли потенциальные проблемы с производительностью и масштабированием? 
  • Есть ли проблемы с безопасностью? Работает ли код с персональными данными пользователя? Валидируются ли входные данные? 

Качество

  • Соответствуют ли изменения требованиям? Уточнить у автора, если есть сомнения.
  • Насколько легко читается код? Понятен ли ход мыслей автора?
  • Понятны ли имена, используемые в коде?
  • Нарушаются ли принципы SOLID?
  • Правильно ли реализована работа с датами и временем? Протестирована ли данная функциональность на работоспособность в разных временных зонах? 

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

  • Достаточно ли юнит тестов к изменениям?
    • Есть ли участки кода, которые не покрыты тестами? Проанализировать, насколько сложно будет их протестировать.
    • Отражает ли именование теста функциональность, которую он покрывает?
    • Протестированы ли контракты (если требуется)?

Релиз

  • Безопасны ли изменения для релиза? Если нет, их нужно выносить либо в отдельную ветку, либо прятать за конфигурационным флагом.