Код ревью

::excerpt

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

Правила:

  • Создавать небольшие пулл-реквесты (ПР, PR) / наборы изменений (CL).
    • деление задач на минимально возможные;
    • использование feature-toggles;
    • использование branch-by-abstraction;
  • CL должен быть завершенным (например, не стоит исключать из CL юнит-тесты).
  • Четко обосновывайте свои заявления, или прямо говорите, что это просто ощущения / безосновательные предпочтения.
    • высказываться ясно и прямо, не заставлять людей додумывать, что имелось в виду;
    • Обучайте. Если что-то предлагаете, то давайте ссылки / доказательства того, что это лучший подход (книги, выступления, статьи, туториалы).
    • Задавайте вопросы в комментариях, запрашивайте уточнения, требуйте аргументированных пояснений
    • Победить должен самый сильный аргумент, тот, что максимально улучшает код.
  • Если предложенное ревьювером решение так же хорошо, как и ваше, примите решение ревьювера. Логика в том, что если вам кажется, что решения одинаковы, то для ревьювера это явно не так. Вам без разницы, а для ревьювера разница есть. С другой стороны, если вы ревьювер и хотите предложить улучшение, но не можете найти четкое обоснование для него - не предлагайте.
  • Хвалить за хороший код!!!
  • В отзывах не должно быть ничего личного, ревью касается только кода
  • Не оценивать и не осуждать автора
  • Не писать запросы (типа, “исправь это, разбей на несколько методов и т.п.”)
  • Без сарказма
  • Не использовать слова всегда/никогда. Исключения возможны.
  • Код не принадлежит никому, избегать выражений “мой/твой/не мой код”
  • Постарайтесь понять точку зрения автора, взглянуть на код его глазами
  • Если обсуждение становится слишком академичным / философским, переносите его в оффлайн
  • Находите способы упростить код, по-прежнему решая поставленную задачу
  • Придерживаться общего для проекта стиля. Да, его нужно выработать, задокументировать и автоматизировать контроль.
  • Давать осмысленные названия переменным, классам, функциям, пакетам.
  • Выровнять уровни абстракции. В том числе в рамках одного файла. Стремиться сохранить линейный формат кода на каждом уровне абстракции. Считайте, что вам удалось, если возможно сделать близкое к реальности предположение о том, что делает функция, прочитав её всего лишь раз сверху вниз.
  • Избегать повторяющихся проверок. В идеале — вытеснить неопределенность в данных на максимально высокий уровень абстракции и сделать некорректное состояние невыразимым на нижележащих.
  • Написать тесты, которые как минимум демонстрируют замысел.
  • Зачищать ненужный код, комментарии, и т.д..
  • Хороший комментарий в коде — это ссылка на документацию или объяснение, почему сделано именно так, а не иначе.
  • Снабжать commit’ы внятными комментариями.
  • Не объединять в одном commit’е разрозненные по смыслу вещи.
  • Делать обзор собственного кода перед созданием pull-request’а.
  • Делать code review. Никакого merge до консенсуального исправления всех значимых замечаний.
  • У проведения код-ревью должен быть очень высокий приоритет, чтобы задачи не застревали на стадии код-ревью.
  • Использовать инструменты статического анализа кода, обеспечения стиля, поиска дефектов, получения статистики по изменениям и т.п.
  • Сертифицировать ревьювером. Лычку “ревьювер” надо заслужить.

Для авторов:

  • Будьте скромны и честны по отношению к коду. Ошибки случаются у всех, процесс код-ревью призван помочь, поддержать и научить.
  • Не принимайте отзывы на свой счет. Ревью - про код, а не про вас лично.
  • Объясняйте, почему существует именно этот код.
  • Следуйте гайдлайнам и правилам оформления кода / коммитов
  • Попытайтесь встать на позицию ревьюера
  • Будьте благодарны за предложенные альтернативы и обсуждайте их в техническом ключе. Используйте все возможности для обучения.

Чеклисты

Smoke test

  • Весь код в коммите относится к какой-либо высокоуровневой и приоритетной задаче
  • Это полная реализация в соответствии со спецификацией
  • Изменения, не относящиеся к задаче, не включены в исходники, а добавлены в виде задач в бэклог
  • Внесенное изменение имеет смысл с функциональной точки зрения
  • Форматирование следует правилам проекта
  • Имена переменных понятны и имеют смысл, магические числа отсутствуют
  • В коде не осталось отладочных выводов в консоль
  • Добавлены только по-настоящему необходимые зависимости
  • Код модульный, удовлетворяет принципу DRY
  • Код компилируется, тесты проходят, линтеры не ругаются, профили проверки кода отрабатывают без ошибок
  • Код безопасен. Публичный и приватный код хорошо разделены
  • Код самодокументирован или внешняя документация обновлена, сообщения в коммитах четкие и ясные
  • Код очень читабелен
  • Добавлены новые юнит-тесты, интеграционные тесты, регрессионные тесты (при необходимости)
  • Мастер-ветка была смержена в ветку разработки и протестирована (при необходимости)
  • Я не могу предложить новых пограничных случаев или возможных источников ошибок для этого кода
  • Я был бы счастлив, если бы авторство этого кода публично было приписано мне
  • Я полностью понимаю, что делает этот код, и каким образом на него повлияют вносимые изменения
  • Я действительно проверил, что этот код выполняет то, что должен выполнять

Соблюдение принципов SOLID

Качество классов Качество классов

Качество методов Качество методов

Защитное программирование Защитное программирование

Переменные

Операторы

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

Общие вопросы

Источники: