Best practices в Code Review
Правильный процесс ревью кода — это процесс контроля. Контроля того, что задача выполнена в полном объёме. Контроля того, что соблюдены общие правила и договорённости. Контроля того, что решение не избыточно и что его легко поддерживать и развивать в будущем.
Для начала будет хорошо задать в своей команды такие вопросы:
Сколько времени занимает ревью кода для средней (сферической в вакууме) задачи?
Как вы минимизируете время ревью?
Как вы определяете, что ревью конкретной задачи сделано правильно?
Коллективное владение кодом
Уменьшение bus-фактора за счет того, что каждый компонент в коде видела и анализировала больше, чем одна пара глаз, и знания не принадлежат одному конкретному разработчику.
Переиспользование кода
Когда несколько человек, решающих разные рабочие задачи и обладающие разным набором знаний о продукте, смотрят код друг друга — они могут увидеть определенные закономерности и либо предложить переиспользовать одно из уже готовых решений, либо выделить текущее в отдельную библиотеку.
Обмен знаниями в команде
У каждого в команде есть чему поучиться. Кто-то хорош в алгоритмах, кто-то разбирается в метапрограммировании, а кто-то джедай MVC. Возможность посмотреть на решения друг друга и задать вопросы — отличный способ поднять технический уровень всей команды.
Обнаружение ошибок
Наиболее очевидная цель — обнаружение ошибок разной степени критичности. Именно об этом в первую очередь думает ваш менеджер, когда вы предлагаете ему заложить определенный процесс времени на code review.
Единообразие проекта
Сколько есть на свете программистов, столько и разных взглядов на то, как именно нужно писать код. Табы и пробелы, файловая структура проекта — при отсутствии консистентности это может сильно усложнить процесс чтения и анализа кода.
Self review
Большую часть фактических ошибок в своем коде автор способен увидеть сам.
Поэтому перед тем, как подключать к ревью своих коллег, стоит самостоятельно один или два раза внимательно просмотреть свой код.
Эта несложная процедура поможет очень сильно сэкономить время и нервы и улучшить отношения с коллегами.
Скорость код-ревью
Скорость команды в целом снижается. Да, если человек не отвечает быстро на код-ревью, он делает другую работу. Тем не менее, для остальной части команды новые функции и исправления ошибок задерживаются на дни, недели или месяцы, поскольку каждый CL ждёт код-ревью и повторного код-ревью.
Медленные ревью также препятствуют очистке кода, рефакторингу и дальнейшим улучшениям существующих CL.
Разработчики начинают протестовать против процесса код-ревью. Если рецензент отвечает только через несколько дней, но каждый раз запрашивает серьёзные изменения, это может быть неприятно и сложно для разработчиков. Часто это выражается в жалобах на то, насколько «строгим» является рецензент. Если рецензент запрашивает те же существенные изменения (изменения, которые действительно улучшают работоспособность кода), но каждый раз реагирует быстро, жалобы обычно исчезают. Большинство жалоб на процесс код-ревью на самом деле решаются путём ускорения процесса
Как быстро выполнять код-ревью?
Если вы не в середине сосредоточенной задачи, то должны сделать код-ревью вскоре после его поступления.
Один рабочий день — максимальное время для ответа (т. е. это первое дело на следующее утро).
Следование этим рекомендациям означает, что типичный CL должен получить несколько раундов ревью (если это необходимо) в течение одного дня.
Скорость и отвлечения
Есть один момент, когда приоритет личной скорости превосходит командный. Если вы в середине сосредоточенной задачи, такой как написание кода, не отвлекайтесь на код-ревью. Исследования показали, что у разработчика может занять много времени, чтобы вернуться в плавный поток разработки после отвлечения. Таким образом, отвлечение от кодирования на самом деле обходится команде дороже, чем просьба к другому разработчику немного подождать код-ревью
Вместо этого дождитесь точки останова в своей работе. Например, после завершения текущей задачи, после обеда, возвращаясь со встречи, возвращаясь с офисной мини-кухни и т. д
Комментарии к MR
Будьте вежливым
Объясните свои рассуждения
Избегайте явных приказов, просто указывая на проблемы и позволяя разработчику решать
Поощряйте разработчиков упрощать код или добавлять комментарии вместо простых объяснений сложности
Хороший пример: модель параллелизма здесь добавляет сложности в систему, и я не вижу какого-то фактического преимущества производительности. Поскольку нет никакого преимущества в производительности, лучше всего, чтобы этот код был однопоточным, а не использовал несколько потоков
Плохой пример:
Почему ты использовали здесь потоки, если очевидно, что параллелизм не даёт никакой выгоды?
Чек-листы и их автоматизация
Примеры вопросов из такого чек-листа: «Есть ли закомментированный код?», «Покрыта ли бизнес-логика тестами?», «Обрабатываются ли все ошибки?», «Вынесены ли строчки в константы?». Запоминайте все вопросы, которые часто возникают у команды на review, и заносите их в список.
Главное — не давать этому чек-листу разрастаться. Думайте о способах автоматизации каждой из перечисленных там проверок — если приложить достаточно усилий, большую часть потребностей вы сможете закрыть. Есть огромное количество инструментов, которые могут помочь в этой работе — линтеры, статические анализаторы, детекторы дублирующегося кода
Архитектурные review
Code Review — не самое подходящее место, чтобы спорить по поводу глобальных архитектурных решений, Код уже написан, задача почти доделана, переписывать все с нуля — слишком затратно. Такие вещи нужно не оставлять на потом, а обсуждать заранее
Как вариант — проведение архитектурного review, защиты своего компонента перед командой либо на словах, либо в виде схемы. Конечно, бывают случаи, когда кого-то осеняет уже во время ревью — тогда это предложение, если оно действительно ценное, стоит задокументировать в issue-трекере и никогда к нему больше не возвращаться
Видеть хорошее
Если видите что-то хорошее в коммите, сообщите разработчику, особенно когда он в лучшем виде решил проблему, изложенную в одном из ваших комментариев. Код-ревью часто просто фокусируются на ошибках, но они также должны поощрять и ценить хорошие практики. С точки зрения менторинга иногда даже более важно сказать разработчику, что именно он сделал правильно, а не где ошибся
Нет идеального
Ключевым моментом здесь является то, что не бывает «идеального» кода — бывает только код получше. Рецензент не должен требовать от автора полировать каждый крошечный фрагментик.
Скорее, рецензент должен сбалансировать необходимость дальнейшего прогресса по сравнению с важностью предлагаемых изменений. Вместо того, чтобы стремиться к идеалу, рецензент должен стремиться к непрерывному улучшению. Коммит, который в целом улучшает ремонтопригодность, читаемость и понятность системы, нельзя задерживать на дни или недели, потому что он не «идеален»
Ограничение размера code review
Работа по расписанию
Можновнедрить таблицу для code review по расписанию
Имя | 9–12 | 12–15 | 15–18 |
Алекс | + | ||
Дэвид | + | ||
Алеша | + |
Кто прав?
Когда разработчик не согласен с вашим предложением, сначала найдите время, чтобы рассмотреть его позицию. Часто он ближе вас к коду и поэтому у него действительно может быть лучшее представление о некоторых аспектах. Есть ли смысл в его аргументе? Есть ли смысл с точки зрения качества кода? Если так, то признайте его правоту, и вопрос отпадёт
Однако разработчики не всегда правы. В этом случае рецензент должен дополнительно объяснить, почему он считает, что его предложение является правильным. Хорошее объяснение демонстрирует как понимание ответа разработчика, так и дополнительную информацию о том, почему требуется изменение
В частности, когда рецензент считает, что его предложение улучшит качество кода, он должен настаивать на нём, если считает, что полученное улучшение качества оправдывает дополнительную работу. Улучшение качества кода, как правило, происходит небольшими шагами
Иногда требуется несколько раундов объяснений, прежде чем оно действительно будет принято. Просто убедитесь, что всегда остаётесь вежливым, и пусть разработчик знает, что вы его слышите, просто не согласны
Ты обиделся?
Рецензенты иногда считают, что разработчик расстроится, если рецензент настаивает на улучшении. Иногда разработчики действительно расстраиваются, но обычно ненадолго, а позже они будут очень благодарны, что вы помогли им улучшить качество их кода
Обычно, если вы вежливы в своих комментариях, разработчики на самом деле не расстраиваются вообще, и беспокойство находится только в голове рецензента
Расстраивает обычно больше стиль написания комментариев, чем настойчивость рецензента в отношении качества кода
На что стоит обратить внимание?
Насколько предлагаемое решение соответствует задаче?
Насколько оно сложно для понимания, поддержки, расширения?
Насколько получившаяся архитектура требовательна к ресурсам и какова сложность реализованных алгоритмов? Можно ли было сделать проще, понятнее, дешевле?
Сделано ли что-то лишнее, что не относится к задаче?
Используем ли мы внешние зависимости и библиотеки? Насколько это оправданно?
Имеет ли решение какие-то недостатки?
Опечатки в коде, магические числа и другие неопределённые константы. Магические строки и другие неожиданные данные, которые программист не учёл при решении задачи.
Проверки на null и другие места, где потенциально можно «выстрелить себе в ногу».
Проверка решения с точки зрения информационной безопасности и так далее.
Тестируемость кода и наличие юнит-тестов. Данный этап проверок также очень важен, ведь он направлен на улучшение той самой пресловутой поддерживаемости кода.
Разработчик везде выбрал хорошие имена? Хорошее имя достаточно длинное, чтобы полностью передать то, чем является или что делает элемент, не будучи настолько длинным, что становится трудно читать.
