<rmcreative>

RSS

Практические приёмы проведения Code review. Конспект

26 февраля

На Temlead Conf Владимир Романько из Лаборатории Касперского рассказывал про Code Review. Я попробовал записать конспект по ходу доклада. Вот что получилось.

На code review жалуются:

  1. Не лучший способ для поиска алгоритмических багов.
  2. Бесполезно потому как выявляются только мелочи.
  3. Тормозит и удорожает разработку.
  4. Демотивирует. Это инструмент для самоутверждения ревьювера, который докапывается.
  5. Плохой инструмент для обмена знаниями.

Неужели всё плохо? Нет! Это мифы в том смысле, что пункты выше не обязаны быть частью ревью и от них можно избавиться.

Не лучший способ для поиска алгоритмических багов

Баги должны обнаруживаться с помощью тестов. Но есть классы алгоритмов которые можно обнаружить только на Code Review:

  1. Уязвимости. Конечно, есть множество инструментов: сканеры, fuzzing-сканеры и статический анализ кода. Но они обнаруживают только если повезёт или работают с сильно специфическими уязвимостями. Ручные тестировщики редко обнаруживают такие баги. Pentest слишком дорог и долог. К тому же, пентестеры начинают всегда с того же code review.
  2. Утечки ресурсов. Автоматизированные тесты не обнаруживают такие ошибки. Ручные тестировщики тоже. Можно гонять нагрузочные тесты, но это долго.
  3. Производительность. Похоже на утечки ресурсов. Медленно и дорого. Как следствие — плохое покрытие. Пример — сброс кеша для всех данных вместо сброса для конкретного ресурса. Найти такое сложно. Найдётся, скорее всего, уже в проде.
  4. Race conditions. Опять же, или review или прод. На проде очень и очень сложно понять, в чём дело. Не воспроизводится.
  5. Все другие ошибки в сложных и трудно-воспроизводимых сценариях.

Бесполезно потому как выявляются только мелочи

  1. Часто проблемы при Code review отражают глубинные проблемы. Если не находится почти ничего — bus factor = 1. Решение — шарить экспертизу.
  2. Программисты не умеют декомпозировать задачи. Жирные куски тяжко ревьюить. Решение — бить на мелкие части.

Тормозит и удорожает разработку

  1. Ревью превращаются в чаты с гигантскими задержками. Решение — два варианта действий:
    1. Устно договориться, фиксировать договорённости.
    2. Молча поправить.
  2. В pull request вообще не те решения архитектурно. Решение: дизайн-ревью (сначала обдумать и обсудить) + короткие циклы обратной связи. Если и потеряем время, то не слишком много.
  3. Обязательные ревьюверы — бутылочное горлышко. Решение: peer review. Сами разработчики инспектируют друг друга, а лид модерирует:
    1. Следит чтобы была внятная аннотация (как и почему решил задачу). Это помогает быстрее разобраться.
    2. Следит за токсичностью. Не допускает.
    3. Добавляет ревьюверов.
    4. В крайнем случае может сам заводить баги.

Демотивирует

  1. Сводить к минимуму общение в виде текста.
  2. Не допускать комментов с наездами, не использовать повелительное наклонение.
  3. Если очень плохой код — не заваливать дефектами. Код нужно сделать достаточно пригодным для master. Слишком много комментов триггерят защитное поведение.
  4. Не комментим стилистические дефекты.
  5. Заводим баги на чужой код.
  6. Критикуешь — предлагай.
  7. Вкусовщину не допускаем. Большинство программистов уверены что они программируют лучше коллег. Решение автора должно всегда побеждать.
  8. Холивары нужно откладывать. Через какое-то время проще договориться.
  9. Демократия для незначительных вопросов. Выносим мелочи на голосование.

Плохой инструмент для обмена знаниями

Нет. Code review:

  1. Переносит культуру написания кода.
  2. Помогает распространять информацию об изменениях в коде.
  3. Позволяет получить навык применения теоретических знаний в боевых условиях с учётом конкретной команды.

Выводы

Да, проблемы есть, но они решаемы.

Комментарии RSS

  1. №11399
    Василий
    Василий 01 марта 2019 г., 16:04:03

    Все проблемы вытекают с психологической стороны этого вопроса. Да, ситуация с "я пишу лучше, чем он" - поголовная. Где-то это действительно так, но где-то просто дело вкуса. К примеру, был человек, который в PHP использовал is_null() вместо === null. Мы просто договорились, что будем везде использовать === null.

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

    Code review требует деликатного подхода. Чтобы он был успешен следует уделить много времени психологии. ИМХО.

  1. Почта опубликована не будет.

  2. Можно использовать синтаксис Markdown или HTML.

  3. Введите ответ в поле. Щёлкните, чтобы получить другую задачу.