Уменьшаем количество ошибок с помощью чек-листа Code Review

http://blog.fogcreek.com/increase-defect-detection-with-our-code-review-checklist-example/
  • Перевод
В нашей статье об эффективных ревью кода мы рекомендовали использовать чеклист. Чеклисты (контрольные списки) — это великая вещь в ревью: они гарантируют, что ревью действительно прошло через вашу команду. Также они способствуют выявлению и решению общих трудностей.

Исследование, проведенное Software Engineering Institute, показывает, что программисты делают 15-20 распространенных ошибок. Добавив такие ошибки в чеклист, вы можете быть уверены, что заметите их в момент появления и поможете от них избавиться надолго.

Чтобы вам было от чего отталкиваться, вот вам список типичных пунктов:

Чек-лист CodeReview


Общее

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

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

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

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

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

Оптимизируйте свой чеклист.

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

Начните использовать и держите в актуальном состоянии

Как правило, любые пункты чеклиста должны быть конкретными и, если возможно, давать вам однозначный ответ на ваш вопрос. Это помогает избежать непоследовательности суждений.
Еще одна хорошая идея — поделиться списком с вашей командой и получить их согласие с его содержимым. Периодически просматривайте и сам чеклист для проверки, что каждый пункт в нем все еще актуален.
Вооружившись хорошим чеклистом, вы можете увеличить количество дефектов, обнаруживаемых во время ревью кода. Это позволит вам улучшить стандарт кодирования и избежать плохого качества ревью.
Чтобы узнать больше о ревью кода, вы можете ознакомиться с видео на нашей странице.
Поделиться публикацией

Похожие публикации

Комментарии 13
    +2
    Спасибо большое. Буду чекать код. Сам пытался составить подобный чеклист, но он все время получался не достаточно полным.
      +1
      •Все ли входные данные проверяются (на корректный тип, длину, формат, диапазон) и декодируются?

      А что такое «декодируются» в данном контексте?
        +1
        Спасибо, я поправил перевод. В оригинале вообще про выходные данные говорится. Здесь речь может идти о том, что данные могут быть получены неизвестно откуда, и их нельзя отдавать в оригинальном виде. Пример: защита от XSS в случае веб-разработки. Но я от этой темы далек, так что буду рад, если кто-то сможет уточнить понятнее и точнее.
          +1
          Это было не замечание, а вопрос :) Думаю, что входные данные надо декодировать, а выходные — кодировать. Но боюсь, что этот пункт говорит о чём-то более глубоком — вроде того, что данные должны преобразовываться в канонический для программы формат в определённых точках программы, а не блуждать по ней во всех возможных форматах одновременно. Скорее всего, я ошибаюсь, поэтому и хотел прояснить.
        0
        Из статьи про эффективные ревью:
        Вам не надо спорить о форматирование и кодстайле. Есть множество инструментов, которые последовательно освещают эти вопросы. Важно, чтобы код был корректным, понятным и поддерживаемым. Конечно, стиль и форматирование — часть этого, но вы должны дать инструментам проверить эти вещи.

        В чек-листе:
        Соответствует ли код вашему стилю написания кода? Обычно это относится к расположению скобок, названиям переменных и функций, длинам строк, отступам, форматированию и комментариям.

        Сами себе противоречат?
          0
          Я полагаю, что в качествен инструментов имелись ввиду две вещи: стандартные соглашения форматирования и автоформатирование в IDE.

          Но на самом деле эти инструменты не исключают необходимость проверки. Очень часто бывает, что человек перед коммитом забыл вызвать функцию автоформатирования в IDE. Если это не контролировать и позволять изменять репозиторий, то следующий человек отформатирует код и в истории будет именно он, что ненамного, но затрудняет поиск реального автора кода.
            0
            Проверка форматирования на стандарт команды может быть и централизованной: например, «плохой» с точки зрения форматирования коммит просто не пройдет в систему управления версиями. Но от странного названия функции это вряд ли спасет.
              0
              Полностью согласен, но на деле не все команды такие вещи используют.

              Автор, скорее всего, хотел привести более-менее универсальный список, который можно адаптировать под нужды.

              Та же проверка входных данных на тип годится только для языков с нестрогой типизацией :)
            0
            Ну вот, чеклист уже можно оптимизировать:) Если на проекте используется автоматизированная проверка форматирования, то как минимум проверку скобок и отступов можно убрать. Но если включить в стиль названия переменных и функций, то автоматизировать это вряд ли удастся: вряд ли инструмент сможет распознать непонятное название переменной.
            0
            Идея хорошая, и я о ней узнал из книги www.amazon.co.uk/Peer-Reviews-Software-Addison-Wesley-Information/dp/0201734850/ref=sr_1_1?ie=UTF8&qid=1423907706&sr=8-1&keywords=software+review могу дополнить своими (С++)

            — все ли деструкторы методов с виртуальными функциями помечены как virtual? (да, это нужно не всегда и порой я вижу ситуации когда в этом нет необходимости)
            — хорошо ли я понимаю кто владеет сырыми указателями и понимаю когда он объект будет удален
            — хорошо ли я понимаю какая функция на каком потоке будет вызвана и что из этого следует
            — есть ли проверки на разыменование указателей, могут ли указатели быть нулевыми?
            — нет ли проблем с кодировками при переходе от string к wstring
              0
              del
                0
                А куда делась статья про эффективность код ревью? И почему?
                  0
                  Там не очень хорошее качество перевода. В комментариях справедливо указали на это, и я спрятал статью в черновики, чтобы отредактировать ее когда-нибудь. Это «когда-нибудь» так и не наступило, к сожалению — так что я вытащил статью из черновика в том же состоянии, как и спрятал ее туда.

                Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                Самое читаемое