Принципы ревью

Технические факты

отменяют личные предпочтения

Руководство по стилю является абсолютным авторитетом

Любая чисто стилевая точка зрения (пробелы, переносы строк и т. д.), которой нет в руководстве по стилю, является вопросом личных предпочтений.

Аспекты разработки программного обеспечения почти никогда не являются чисто стилевым вопросом или личным предпочтением

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

Этапы прохождения код ревью

Ответить на вопросы:

1. Имеют ли смысл изменения?
2. На сколько хорошо и понятно составлено описание?
3. Какая бизнес и/или техническая цель изменений?

Общий взгляд на изменения

• Проверьте название PR и его описание
• Определите общую идею изменений, их дизайн и функциональность

Определите основные части изменений

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

Проверяйте логически связанные части

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

Дизайн изменений

Самая важная вещь в ревью - это оценка общего дизайна PR.

• Имеет ли смысл взаимодействие различных частей кода в PR?
• Относится ли изменение к вашей кодовой базе?
• Хорошо ли он интегрируется с остальной частью системы?
• Стоит ли добавить эту функциональность именно сейчас?

Функциональность

• Функционал делает то, что задумал разработчик?
• Реализация функционала полезна для пользователей? («Пользователи» - это, как правило, как конечные пользователи, когда на них повлияют изменения, так и разработчики, которым придется «использовать» этот код в будущем)
• Также стоит подумать, не ведется ли параллельно разработка того, которая будет конфликтовать с текущим PR

Сложность

• Изменения выглядят сложнее, чем они должны быть?
• Автор добавил избыточную функциональность?
• Встречается чрезмерное проектирование?

«Слишком сложный»

обычно означает, что «код не может быть быстро прочитан и понят»

Чрезмерное проектирование

- это реализация более универсального кода, чем нужно, или добавленная функциональность, которая на данный момент системе не нужна

Тесты

• Проверьте наличие юнит , интеграционных или e2e тестов, в зависимости от изменений
• Тесты должны быть в том же PR, что и измененный код
• Тесты не должны тестировать сами себя
• Тесты должны описывать бизнес логику
• Тесты должны покрывать конкретные части функционала

Помните, что тесты

- это код, который необходимо поддерживать

Нейминг

На сколько информативны имена переменных, методов и т.д.?

Хорошее имя

достаточно длинное, чтобы полностью передать, что представляет из себя предмет или что делает функция, но не настолько длинное, чтобы его стало трудно читать

Комментарии

В комментариях нет необходимости, если код простой и самодокументируемый

(но так происходит не всегда)

Внимание к комментариям

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

Стиль

• Убедиться в соответствии руководству по стилю
• Если изменения не соответствуют, но улучшают стиль, то обсудить с командой и задокументировать
• Не блокировать мерж PR, основываясь только на личных предпочтениях
• Автор не должен включать большие изменения по стилю кода вместе с функциональными изменениями. Для этого должен быть отдельный PR

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

• Если PR изменяет способ сборки, тестирования, запуска или работы кода, убедитесь, что автор также обновил документацию
• Если в PR удаляется часть функционала, подумайте, следует ли также почистить документацию
• Если документация отсутствует, попросите создать её

Построчное чтение

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

• Если вы понимаете код, но не чувствуете себя достаточно компетентным для проверки какой-либо части, то пригласите в PR проверяющего, кто обладает соответствующей квалификацией

Замечать хорошее

Если вы видите что-то хорошее в PR, сообщите об этом автору. На код ревью часто просто фокусируются на ошибках, но также полезно отмечать хорошие стороны разработки. Иногда с точки зрения наставничества даже более ценно рассказать разработчику, что он сделал правильно, чем рассказать ему, что он сделал неправильно

Итого

Делая код ревью, убедитесь, что:

• Код хорошо спроектирован

• Функциональность действительно необходима пользователям кода и приложения

• Любые изменения в интерфейсе разумны и выглядят хорошо

• Любое асинхронное программирование выполняется в правильном порядке и не порождает необработанных исключений

• Код не сложнее, чем должен быть

• Разработчик не реализует вещи наперёд без четкого понимания и требований к их использованию

• Код покрыт тестами

• Тесты хорошо продуманы, корректны и полезны

• Разработчик использовал подходящие имена для всего

• Комментарии понятны и полезны, и в основном объясняют причину, а не то, что сделано

• Код надлежащим образом документирован

• Код соответствует нашим руководствам по стилю