Попса

популярный светский альманах

Тег: рецензия

Базовый чеклист для ревью кода

Ревью кода

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

Качество ревью можно повысить при помощи карты контрольных проверок. Данный чеклист взят с хабра и является отправной точкой. Какие пункты вам нужны, а какие нет — решайте сами.

Общее
Работает ли код? Выполняет ли он свои прямые обязанности, корректна ли логика, и т. д.
Легок ли код для понимания?
Соответствует ли код вашему стилю написания кода? Обычно это относится к расположению скобок, названиям переменных и функций, длинам строк, отступам, форматированию и комментариям.
Есть ли в ревью избыточный или повторяющийся код?
Является ли код независимым, насколько это возможно?
Можно ли избавиться от глобальных переменных или переместить их?
Есть ли закомментированный код?
У циклов есть установленная длина и корректные условия завершения?
Может ли что-то в коде быть заменено библиотечными функциями?
Может ли быть удалена часть кода, предназначенного для логгирования или отладки?

Безопасность
Все ли входные данные проверяются (на корректный тип, длину, формат, диапазон) и кодируются?
Обрабатываются ли ошибки при использовании сторонних утилит?
Выходные данные проверяются и кодируются (прим. пер.: например, от XSS)?
Обрабатываются ли неверные значения параметров?

Документация
Есть ли комментарии? Раскрывают ли они смысл кода?
Все ли функции прокомментированы?
Есть ли какое-то необычное поведение или описание пограничных случаев?
Использование и функционирование сторонних библиотек документировано?
Все ли структуры данных и единицы измерения описаны?
Есть ли незавершенный код? Если есть, должен ли он быть удален или помечен маркером типа «TODO»?

Тестирование
Является ли код тестируемым? Например, он не должен содержать слишком много зависимостей или скрывать их, тестовые фреймворки должны иметь возможность использовать методы кода, и т. д.
Есть ли тесты и если есть, то достаточны ли они? Например, они покрывают код в нужной мере.
Юнит-тесты на самом деле проверяют, что код предоставляет требуемую функциональность?
Все ли массивы проверяются на «выход за границы»?
Может ли любой тестирующий код заменен с использованием существующего API?

Источник

О рецензировании

fire

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

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

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

Проблема в том, что все положительные моменты перекрываются формированием у исполнителей особого мировоззрения «если я ошибся, рецензент заметит». Интересно, я все проверки предусмотрел? Да вроде все, но если что-нибудь упустил, рецензент укажет, а если не укажет — что поделаешь, он же опытней.

Нет.

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

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

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

Я считаю рецензии потерей времени.