Best practices в Code Review +13


Правильный процесс ревью кода — это процесс контроля и итеративного улучшения качества продукта.

Контроля того:
1) что задача выполнена в полном объёме и соблюдены все критерии приемки.
2) что соблюдены общие правила и договорённости.
3) что решение не избыточно, его легко поддерживать и масштабировать в будущем.

Для начала будет хорошо задать своей команде такие вопросы:

  1. Сколько времени занимает ревью кода для средней (сферической в вакууме) задачи?

  2. Как вы минимизируете время ревью?

  3. Как вы определяете, что ревью конкретной задачи сделано правильно?

Командное владение кодом

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

Переиспользование кода

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

Обмен знаниями в команде

У каждого в команде есть чему поучиться. Кто-то хорош в алгоритмах, кто-то разбирается в базах данных, а кто-то мастер MVI. Возможность посмотреть на решения друг друга и задать вопросы — отличный способ поднять технический уровень всей команды.

Обнаружение ошибок

Наиболее очевидная цель — обнаружение ошибок разной степени критичности. Именно об этом в первую очередь думает ваш PM, когда вы предлагаете ему заложить определенный процент времени на code review.

Единообразие проекта

Сколько есть на свете программистов, столько и разных взглядов на то, как именно нужно писать код. Табы и пробелы, файловая структура проекта — при отсутствии консистентности это может сильно усложнить процесс чтения и анализа кода.

Рефлексия или self review

Большую часть ошибок в своем коде автор способен увидеть сам. 
Поэтому перед тем, как подключать к ревью своих тиммейтов, стоит самостоятельно хоть раз внимательно просмотреть свой код. 

Эта простая процедура поможет сильно сэкономить время и нервы, а также улучшит отношения с коллегами.

Скорость code review

  1. Скорость команды в целом снижается. Да, если человек не отвечает быстро на код-ревью, он делает другую работу. Тем не менее, для остальной части команды новые функции и исправления ошибок задерживаются на дни, недели или месяцы, поскольку каждый PR ждёт код-ревью и повторного код-ревью.

  2. Медленные ревью также препятствуют очистке кода, рефакторингу и дальнейшим улучшениям существующих PR.

  3. Разработчики начинают протестовать против процесса код-ревью. Если рецензент отвечает только через несколько дней, но каждый раз запрашивает серьёзные изменения, это может быть неприятно и сложно для разработчиков. Часто это выражается в жалобах на то, насколько «строгим» является рецензент. Если рецензент запрашивает те же существенные изменения (изменения, которые действительно улучшают работоспособность кода), но каждый раз реагирует быстро, жалобы обычно исчезают. Большинство жалоб на процесс код-ревью на самом деле решаются путём ускорения процесса

Как быстро выполнять код-ревью?

Если вы не в середине сосредоточенной задачи, то должны сделать код-ревью вскоре после его поступления.

Один рабочий день — максимальное время для ответа (т. е. это одно из первых задач на следующее утро).

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

Скорость и отвлечения

Есть один момент, когда приоритет личной скорости превосходит командный. Если вы в середине сосредоточенной задачи, такой как написание кода, не отвлекайтесь на код-ревью. Исследования показали, что у разработчика может занять много времени, чтобы вернуться в плавный поток разработки после отвлечения. Таким образом, отвлечение от кодирования на самом деле обходится команде дороже, чем просьба к другому разработчику немного подождать код-ревью

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

Комментарии к PR

  • Будьте вежливым

  • Объясните свои рассуждения

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

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

Хороший пример: Использование этой библиотеки в данном случае является лишним, так как она добавляет большое количество overhead'a, т.е функционала которым мы пока не пользуемся. Думаю в данном случае было бы целесобразно взять только нужную часть и использовать ее в новом модуле.

Плохой пример:
Зачем ты запилил эту либу? Откуда вообще нашел ее? Советую выпилить и самому сделать.

Чек-листы и их автоматизация (DevOps)

Примеры вопросов из такого чек-листа:
«Есть ли закомментированный код?»,
«Покрыта ли бизнес-логика тестами?»,
«Обрабатываются ли все ошибки?»,
«Вынесены ли строчки в константы?».
Запоминайте все вопросы, которые часто возникают у команды на review, и заносите их в список.

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

Например: SonarQube

Архитектурные встречи

Code Review — не самое подходящее место, чтобы спорить по поводу глобальных архитектурных решений. Код уже написан, решение выкачено, задача почти доделана, переписывать все с нуля — слишком затратно и зачастую не оправдано. Такие вещи нужно не оставлять на потом, а обсуждать заранее.
Как вариант — проведение архитектурной встречи, защиты и аргументации своего решения для команды, либо на словах, либо в виде схемы. Зачастую, бывают случаи, когда кого-то осеняет уже во время ревью — тогда это предложение, если оно действительно ценное, стоит задокументировать в issue-трекере и вернуться к нему по мере необходимости.

Видеть хорошее

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

Нет идеального

Главным point'ом здесь является то, что не бывает «идеального» кода — бывает только код «получше». Не стоит требовать от автора полировать каждый крошечный кусок или фрагмент.

Скорее, рецензент должен сбалансировать необходимость дальнейшего прогресса по сравнению с важностью предлагаемых изменений. Вместо того, чтобы стремиться к идеалу, команда должна стремиться к непрерывному улучшению. Коммит, который в целом улучшает масштабируемость, читаемость и простоту системы, нельзя задерживать на дни или недели, потому что он не «идеален»

Ограничение размера code review 

https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/
https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/

Работа по расписанию

Можно внедрить таблицу для code review по расписанию 

Имя

9-12

12-15

15-18

Алекс

+

 

 

Дэвид

 

+

 

Алеша

 

 

+

Кто прав?

Когда разработчик не согласен с вашим комментарием, найдите время и рассмотрите его позицию. Зачастую он ближе вас к коду и поэтому у него действительно может быть более объективное представление. Есть ли смысл в его аргументе с точки зрения качества кода или бизнес решения? Если так, то признайте его правоту, и вопрос отпадёт

Однако разработчики не всегда правы. В этом случае тот кто проводит review должен объяснить, почему он считает, что его предложение является правильным. Хорошее объяснение демонстрирует как понимание ответа разработчика, так и дополнительную информацию о том, почему требуется изменение

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

Иногда требуется несколько раундов объяснений, прежде чем оно действительно будет принято. Просто убедитесь, что всегда остаётесь вежливым, и пусть разработчик знает, что вы его слышите, просто не согласны

Внутренние обиды

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

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

Расстраивает обычно стиль написания комментов , нежели настойчивость рецензента в отношении качества кода

На что стоит обратить внимание?

  1. Насколько предлагаемое решение соответствует бизнес требованиям? 

  2. Насколько оно сложно для понимания, изменения, масштабирования? 

  3. Можно ли было сделать легче, понятнее, дешевле, быстрее и проще? Насколько получившаяся архитектура требовательна к ресурсам и какова сложность алгоритмов?

  4. Сделано ли что-то лишнее, что не относится к задаче? Насколько это оправданно?

  5. Используем ли мы внешние зависимости? Насколько это оправданно?

  6. Имеет ли решение критические недостатки? 

  7. Опечатки в коде, magic numbers и другие неопределённые константы, которые разработик не учёл при решении задачи. 

  8. Потенциальные места где можно «выстрелить себе в ногу».

  9. Тестируемость кода и наличие юнит-тестов. Данный этап проверок также очень важен, ведь он направлен на улучшение поддерживаемости кода.

  10. Разработчик везде выбрал правильный naming?
    Хорошие naming должен полностью передать то, чем является или что делает обьект, не будучи настолько длинным, что становится трудно читать.




Комментарии (18):

  1. dopusteam
    /#23905043

    А что такое CL?

    Хороший пример: модель параллелизма здесь добавляет сложности в систему, и я не вижу какого-то фактического преимущества производительности. Поскольку нет никакого преимущества в производительности, лучше всего, чтобы этот код был однопоточным, а не использовал несколько потоков

    Плохой пример:Почему ты использовали здесь потоки, если очевидно, что параллелизм не даёт никакой выгоды?

    Мне кажется, оба примера не очень. В первом примере субъективное мнение (я не вижу) и нет вопроса. Ну т.е. тут неплохо было бы узнать мнение автора кода, почему сделано так, а не сразу писать, что так не нужно. Вот плохой пример чуть подтюнить до Мне кажется, потоки тут добавляют излишнюю сложность. Может лучше без них, или есть какие то причины для их использования? и, по моему мнению, будет хорошо

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

    Выглядит слишком категорично. Ситуации разные бывают, иногда лучше переделать заново и в будущем стараться такого не допускать. Есть такая ловушка, не помню названия, в духе "уже слишком много времени потратил на это, нельзя потерять результаты работы". Приведу пример из опыта, когда приняли реквест, хотя все знали, что он не готов до конца, но слишком долго тянули его и жалко было выбрасывать. После принятия потратили ещё +- столько же времени на правки глубоколежащих багов, внесённых реквестом. При этом часть багов вывалилось после тестирования, часть были выброшены как "не баг, а фича".

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

    Звучит интересно, конечно)

    • Vadem
      /#23905517 / +3

      А что такое CL?

      Возможно имеется ввиду change list - то же самое, что и pull request.

      • timkaopensoul
        /#23909409

        верно, поправил везде на PR чтобы не путать читателей)

  2. alexg-nn
    /#23905245

    Коллективное владение кодом

    Речь наверное всё же про увеличение бас-фактора, а не про его уменьшение. Больше - лучше.

    • timkaopensoul
      /#23909487

      Фактор автобуса проекта — это мера сосредоточения информации среди отдельных членов проекта; фактор означает количество участников проекта, после потери которых (в оригинале — «попадания» которых под автобус или грузовик, варианты: увольнения, заболевания, рождения у них ребёнка, наступления несчастного случая и других форс-мажорных обстоятельств) проект не сможет быть завершён оставшимися участниками.

      • alexg-nn
        /#23909497

        Это я знаю. У вас в статье написано, фактически, что уменьшение бас-фактора это хорошо. Я указал на то, что уменьшение бас-фактора - это плохо.

  3. Dasfex
    /#23905275

    Не очень понял табличку про ограничение размера code review. Это какая-то зависимость кол-ва комментариев от кол-ва строк? Почему она обратная?

    Ещё немножко критики по поводу ревью по времени. Обычно, приглашая кого-то посмотреть код, ты делаешь это из соображений, что человек знает ситуацию чуть лучше тебя/других в конкретном месте, где делается новый pr. Ну и смена ревьюера в течение дня может негативно повлиять на некоторые комментарии(2й ревьюер банально не поймёт, что имел в виду 1й). Учитывая, что в большинстве систем контроля версий есть block merge или какой-нибудь request changes получается не очень эффективное привлечение коллег. Ещё, во времена удалёнки и свободных графиков, такой подход может привести к неравномерному распределению нагрузки.

    • navferty
      /#23905529 / +1

      Это какая-то зависимость кол-ва комментариев от кол-ва строк? Почему она обратная?

      Практика показывает, что когда ты просишь коллегу посмотреть merge request на 50-100 строк, он сможет погрузиться в предлагаемые изменения, и наверняка оставит комментарии со своими предложениями.

      Начиная с какого-то объёма, погружаться в содержимое MR становится всё труднее, и в конечном счёте его либо завернут с просьбой вливать по частям, либо вольют и так, без замечаний (если есть доверие к автору и подстраховка в виде CI и тестовой среды, например).

      • Dasfex
        /#23907581

        Тогда понятно. Но какой-то очень странный график, как заметил@WASD1. И непонятно, какую мысль автор хотел донести. Не делать большие пр? Ну наверн надо бы написать это.

    • WASD1
      /#23907223

      Похоже по оси Х - число дописаных строк кода (размер диффа или что-то похожее), а по оси Y - число написанных строк, при обсуждении Review Request.

      Хотя странно, по моим ощущениям это вроде должен быть "купол" или "колокол", с максимумом в районе 50-100 строк.

      • timkaopensoul
        /#23909405

        Есть и такая методология, называется Trunk Based Development )

  4. saboteur_kiev
    /#23905943 / +1

    Пропущен один из важнейших моментов код ревью, а именно DevOps составляющая.

    Именно на код ревью, а точнее на merge request(pull request) можно навешать множество автоматических проверок, которые нет смысла вешать на каждый коммит. А вот на попытку смержить, уже можно и билд с полным набором тестов, и анализаторы прикрутить, без которых мерж нельзя будет сделать.

    • timkaopensoul
      /#23909397

      Все верно, devops упомянут в статье, но согласен что можно развернуть эту тему более обширно.

  5. peakle
    /#23906231

    Статья хорошая, но есть вопрос :)

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

    Почему стоит задокументировать и не возвращаться? Можно поподробнее раскрыть предложение, что имелось в виду.

    • ivanych
      /#23907687 / +1

      Явно имелось в виду "принять решение, задокументировать и больше не переливать из пустого в порожнее". Но походу для автора русский язык не родной.

      • timkaopensoul
        /#23909387

        Все верно, в данном случае имеется ввиду вернуться к ней по мере необходимости в соответствии с планированием ресурсов и времени для тех.долга. Спасибо что обратили внимание, поправил.

  6. AntonSazonov
    /#23907107

    Для начала будет хорошо задать в своей команды такие вопросы:

    Писал в ЛС, но сообщения, как я понял, не доходят до адресата.

    Привет "гениальным" разработчикам хабра!