Автор: Джереми Миккола

«Программы должны быть написаны для того, чтобы их могли читать люди, и лишь случайно для того, чтобы машины могли их выполнять» — Гарольд Абельсон.

Здесь, в Thumbtack, мы проверяем код каждого изменения, которое объединяем. Обзоры кода имеют много преимуществ: они уменьшают количество ошибок и проблем с безопасностью, делают код более читабельным и помогают делиться контекстом кода. Они даже могут быть полезны для наставничества, поскольку служат практическим обучением передовому опыту.

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

Как автор обзора кода

Эффективность и действенность процесса проверки кода зависит как от автора кода, так и от рецензентов кода. Вы не получите многого от проверки кода для вашего изменения, если вы попросите проверить код от кого-то, кто не понимает кодовую базу.

Перед отправкой кода на проверку

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

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

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

Как отправить код-ревью

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

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

Если вам нужен обзор кода от кого-то, кто может этого не осознавать (или по какой-то причине вам нужен быстрый обзор), обычно можно отправить личное сообщение этому человеку.

Как отвечать на комментарии

Помните, что комментарии предназначены только для того, чтобы сделать код лучше, а не для личных нападок (хотя иногда это может так показаться!). Часто занятые рецензенты будут очень кратки в своих комментариях — это не значит, что они сварливы, это просто означает, что они пытаются быть эффективными. Некоторые рецензенты используют в своих комментариях различные сокращения. Например, начало комментария с «nit:» указывает на то, что комментарий является «придиркой» второстепенного стиля. Комментарий, говорящий просто «100ch», означает, что строка длиннее 100 символов и должна быть перенесена.

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

Когда вы вносите изменения в ответ на комментарии, помечайте комментарий как «Готово». Это полезно для вас, чтобы отслеживать, какие комментарии у вас есть, а какие еще не рассмотрены (особенно если вас прерывают), а рецензенту также полезно видеть, был ли уже рассмотрен комментарий или нет. Если вы не вносите изменения в соответствии с запросом, обязательно объясните, почему в ответ на комментарий.

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

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

Как рецензент

Когда вы просматриваете код, всегда помните о доброте. Вы критикуете ремесло человека, которое связано с его личностью. То, что кажется вам объективным наблюдением, может показаться личным нападением на автора кода, если оно сформулировано плохо.

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

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

Обязательно своевременно отвечайте на код-ревью. Практика просмотра кода не должна происходить за счет значительного замедления скорости внесения изменений.

Делайте регулярные обзоры несколько раз в день. Делая слишком много сразу, ваши обзоры будут менее эффективными (через час у вас будет косоглазие). Кроме того, полезно получить несколько циклов ответа в течение дня. Тем не менее, вам нужно сбалансировать быстрое реагирование и отвлечение от другой работы. Лично я считаю полезным проводить код-ревью, когда я уже отвлечен (например, после обеда или совещания).

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

Практические заметки

На практике полезно пропускать проверки кода при отмене уже внесенных изменений. Это позволяет инженерам быстро исправлять сломанные сборки и производственные проблемы, не дожидаясь проверки кода. Также полезно определить набор критериев, когда чрезвычайная ситуация требует отправки кода без проверки кода. Например, может быть нормально выдвигать экстренное исправление, когда страница не отображается в половине случаев, но не тогда, когда предупреждение регистрируется каждые несколько минут. Дело не в том, где именно вы проводите линию; дело в том, что у вас должна быть нарисована какая-то линия.

Когда в обзоре кода участвуют несколько рецензентов, один из рецензентов часто хочет указать, что у них нет никаких изменений, которые они хотели бы запросить, но они все еще хотят, чтобы кто-то еще проверил код. Мы используем для этого систему «+1» и «+2» — оценка кода «+2» означает, что его можно объединить, а оценка «+1» просто означает, что они не возражают. Это особенно полезно, когда есть разные рецензенты для разных частей рецензирования (например, один рецензент просматривает SQL, а другой просматривает CSS). Первый рецензент своего раздела ставит ему «+1», а второй рецензент (увидев, что первый рецензент уже одобрил его статью) ставит ему «+2». Условия гонки могут привести к дублированию «+1», но это легко решается автором обзора, заставляющим рецензента поставить «+2».

Мы также разрешаем блокировать отправку изменений в качестве защиты от случайного нажатия чего-то сломанного. Ответив «-1», рецензент может заблокировать отправку проверки кода до тех пор, пока он не удалит свой «-1» (даже если проверка кода имеет «+2»). Интересно, что чаще всего эту функцию в Thumbtack используют авторы обзоров кода. Они применяют «-1» к своим собственным изменениям, чтобы напомнить себе, что нельзя объединяться до тех пор, пока не произойдет какое-либо другое событие (например, миграция базы данных).

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

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

По возможности используйте автоматизированные инструменты для поиска проблем, а не вручную. Автоматические проверки, такие как линтеры, лучше ручных проверок по двум причинам: во-первых, они экономят ваше время как рецензента, а во-вторых, они никогда не пропускают проблемы, которые ищут. Для проектов Go сбой сборки, если код не отформатирован в соответствии с gofmt, означает, что почти никогда не тратится время на обсуждение правильного стиля. Никто не пытается спорить с CI-сервером. Если есть какая-либо опасная ошибка, которую легко совершить (например, использование строковой функции, которая не является многобайтовой), подумайте о том, чтобы найти линтер или написать собственный скрипт (мы используем и то, и другое) для поиска этих ошибок. Их, как правило, очень легко не заметить как людей-рецензентов.

Что искать

Полезно иметь контрольный список того, что нужно искать в обзоре кода, потому что очень легко забыть подумать о чем-то важном, особенно когда разные обзоры кода заставляют вас переключаться между разными областями контекста. Очень малая часть этого контрольного списка должна применяться к любой отдельной проверке кода (если только это не довольно большое изменение), но некоторые подмножества применимы почти к каждой проверке кода.

Этот список относится как к рецензенту, так и к автору рецензии.

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

Это правильное изменение?

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

Спроси себя:

  • Должна ли вообще произойти эта перемена? Этот вопрос более актуален для автора, чем для рецензента, но рецензенту все равно полезно его обдумать. Если (как рецензент) вы не можете сказать, потратьте некоторое время на понимание цели изменения, прежде чем проверять его.
  • Какие еще существуют способы сделать то же самое? Есть ли лучший способ сделать то же самое? Является ли написание кода правильным способом решения этой проблемы? Есть некоторые классы проблем, которые можно решить с помощью кода, но у которых есть лучшие/дешевые решения.

Косметические проблемы

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

Спроси себя:

  • Соответствует ли он руководству по стилю языка?
  • Есть ли более приятный способ выразить определенные детали в коде, чтобы сделать их чище или более идиоматичными? Идиомы часто существуют по уважительной причине.
  • Нужно ли обновлять комментарии к документам с учетом нового набора аргументов и их типов?
  • Если в коде есть TODO, объясняют ли они, что нужно сделать? Стоит ли делать это сейчас?
  • Есть ли опечатки?
  • Имеют ли смысл имена функций и переменных?
  • Объясняет ли сообщение фиксации почему вносится изменение? Пойму ли я объяснение через год? Есть ли какая-либо документация или тикет Jira, на который он может ссылаться?

Функциональность и корректность

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

Общая правильность

Спроси себя:

  • Действительно ли код делает то, что заявлено? Достаточно ли хорошо определено, что должен делать код, чтобы решить это?
  • Правильно ли он справляется с неверным вводом, ошибками, тайм-аутами или другими крайними случаями?
  • Делают ли какие-либо клиенты интерфейса предположения, которые больше не верны? Если контракт интерфейса изменяется, весь код, который зависит от него, должен быть проверен на соответствие новому контракту. (Контракт интерфейса означает больше, чем просто имя функции и тип данных параметров, он также включает поведение и побочные эффекты функций.)
  • Работают ли новые характеристики производительности этого кода для всех вызывающих абонентов?
  • Будет ли этот код слишком дорогим (либо в компьютерном времени, либо в реальных деньгах!) для запуска?
  • Если операция не выполняется на полпути, остается ли система в состоянии, когда операцию можно безопасно повторить?

Безопасность

Безопасность — важная составляющая функциональной корректности!

Спроси себя:

  • Зарегистрированы ли какие-либо пароли или токены аутентификации в коде? (Не делай этого!)
  • Ограничен ли доступ к данным для чтения/записи правильным набором клиентов?
  • Существует ли проверка на стороне сервера в дополнение к проверке на стороне клиента?
  • Может ли код быть уязвим для распространенных векторов атак, таких как межсайтовый скриптинг или атаки по времени?
  • Есть ли вероятность случайного ввода пароля?
  • Хранятся ли пароли как что-либо кроме надежного (например, bcrypt) хэша?
  • Есть ли входные данные, которые могут нарушить данную функцию? Если да, то разумно ли ожидать входных данных, которые ломают вещи?
  • Подвергаются ли какие-либо очень дорогостоящие операции клиентам, что позволяет им создавать большую нагрузку на сервер?
  • Что-нибудь мешает клиенту пытаться подобрать пароли?

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

Системы с параллелизмом

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

Спроси себя:

  • Является ли этот код безопасным для одновременного вызова или он выполняет параллельные вызовы вещей, которые небезопасно вызывать одновременно?
  • Приняты ли соответствующие замки? Может ли состояние гонки вызвать разблокировку перед блокировкой?
  • Может ли он войти в тупик или активную блокировку?
  • Является ли доступ к данным атомарным?
  • Является ли количество ресурсов (например, потоков, соединений с базой данных), которые он может использовать, ограниченным или неограниченным?

Опять же, этот список не претендует на полноту. Просто не забудьте подумать о том, верна ли логика с учетом параллелизма.

Асинхронная работа

Если вы выполняете какую-то работу асинхронно (например, вы обрабатываете задания в очереди SQS), эти вопросы, вероятно, покажутся вам знакомыми.

Спроси себя:

  • Обрабатываются ли ошибки в обработке? Узнаю ли я, если вся работа потерпит неудачу?
  • Есть ли способ узнать, сколько времени требуется для обработки части работы? Есть ли способ увидеть текущий размер очереди?
  • Неудачная работа повторяется? Должно ли это быть?
  • Есть ли ограничения на количество повторных попыток? Что происходит, когда он терпит неудачу слишком много раз?

Сетевые звонки

Спроси себя:

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

HTTP-серверы

Спроси себя:

  • Есть ли у него время ожидания выключения больше, чем максимальное время, разрешенное для операций?
  • Есть ли у него тайм-ауты клиента и ограничения на размер запроса?
  • Отвечает ли он правильными HTTP-статусами? Он когда-нибудь делал что-то глупое, например, возвращал статус 200, когда на самом деле была ошибка?
  • Возвращает ли вызывающему абоненту соответствующие сообщения об ошибках? Убедитесь, что службы не возвращают внутреннюю информацию третьей стороне, но также убедитесь, что существует достаточно подробностей, чтобы другие внутренние клиенты могли надлежащим образом обрабатывать ошибки.
  • Должен ли он передавать данные через TLS?

Использование базы данных

Спроси себя:

  • Ограничена ли скорость записи для пакетных заданий, чтобы не перегружать систему базы данных?
  • Если база данных может ограничить скорость вашего запроса, отступите ли вы и повторите попытку?
  • (Предположим, что реплики базы данных доступны только для чтения) Чувствителен ли код к задержке репликации? Справляется ли он с большими, чем обычно, задержками?
  • Когда изменение вводит новое хранилище данных, есть ли резервные копии?
  • Обеспечивает ли код целостность данных?

Если производительность имеет какое-либо значение (это не всегда):

  • Будут ли запросы эффективными?
  • Используют ли они индекс для поиска данных?
  • Ограничено ли количество строк, которые ядро ​​базы данных должно сканировать, чтобы найти результирующий набор, разумным числом?
  • Ограничен ли размер результирующего набора разумным числом?
  • Существуют ли шаблоны запросов N+1, которые можно исправить с помощью пакетного запроса?
  • Бывают ли случаи, когда один и тот же объект загружается дважды?
  • Достаточно ли производительны запросы для случая с самым большим набором данных? Подумайте о самом активном пользователе на сайте — вы определенно хотите убедиться, что он получает хороший опыт, но он часто работает с самым большим набором данных.
  • Удерживаются ли какие-либо блокировки дольше нескольких миллисекунд?

Проблемы, связанные с языком

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

Если вы пишете на Go, спросите себя:

  • Он отправляет/получает с нулевого канала?
  • Пытается ли он записать на карту nil? Это особенно важно проверять при добавлении нового поля в структуру, содержащую карту. Все, что создает эту структуру, инициализирует карту?
  • Восстанавливается ли он после паники в горутинах? (Они убивают весь процесс, если не обработаны.)
  • Страдает ли он алиасингом указателя в циклах?

Дизайн, качество и ясность кода

О том, как проектировать код, написана целая библиотека книг, поэтому, конечно, этот список не охватит всего.

Спроси себя:

  • Можно ли и нужно ли разбивать это изменение на более мелкие изменения?
  • Имеет ли смысл структура кода? Разделены ли заботы?
  • Есть ли хорошие интерфейсы в нужных местах? Эти интерфейсы хорошо разработаны?
  • Обеспечивает ли общий дизайн эффективную реализацию?
  • Есть ли преждевременная оптимизация или преждевременное обобщение?
  • Изобретает ли он заново какие-либо колеса (например, формат ведения журнала, систему кэширования), которые не нужно изобретать заново?
  • Затрудняет ли что-либо это дальнейшее развитие?
  • Использует ли новый код интерфейсы, которые не предназначались для использования/использования таким образом?
  • Используются ли при этом новейшие методы, устаревшие шаблоны или устаревшие библиотеки?
  • Соответствует ли новый код структуре существующего кода? Это в правильном пространстве имен?
  • Позволяет ли дизайн эффективное тестирование и хорошо ли протестирован код? Существуют ли интеграционные тесты между вызывающими объектами и этим кодом?
  • Нужны ли в новом или существующем коде какие-либо комментарии, поясняющие неочевидные детали взаимодействия кода?
  • Насколько сложно будет это удалить? Это один из способов проверить, не слишком ли много связи.
  • Достаточно ли информации для отслеживания, чтобы понять, что происходит, когда что-то идет не так (например, когда система начинает работать слишком медленно)?
  • Если что-то пойдет не так, безопасно ли откатить это изменение или оно создает состояние, которое делает это небезопасным? Является ли изменение безопасным для канарейки? Например, не добавляйте конечную точку и то, что вызывает эту конечную точку, в одном и том же развертывании.

Большая картина

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

Спроси себя:

  • Есть ли конфликты с другими предстоящими или текущими изменениями?
  • Следует ли написать или обновить какую-либо документацию в результате этого изменения? Сюда входит не только документация для разработчиков, но и документация для остальной части организации и даже общедоступная документация (например, страницы справки).
  • Должны ли люди быть уведомлены об этом изменении? Сколько времени им нужно дать, прежде чем изменение будет развернуто?
  • Решаем ли мы основную проблему или это обходной путь для более глубокой проблемы?
  • Следует ли провести рефакторинг перед внесением этого изменения?
  • Соответствует ли уровень структуры и полировки цели этого кодекса? Тратит ли это много кода на создание абстракций, когда это одноразовый код, или мы должны потратить больше времени на создание хорошей структуры для него, поскольку это, вероятно, будет существовать в течение длительного времени? Выглядит ли это как «временный» код, который может прожить долгое время? Если вы извините краткий момент мыльной упаковки: изменить код позже часто намного, намного сложнее, чем вы думаете, поэтому почти всегда лучше начинать со структуры, у которой нет серьезных проблем. Я не имею в виду, что чем больше структуры, тем лучше; Я имею в виду, что вы должны избегать сознательного совершения дизайнерских грехов с намерением искупить их позже.

Вывод

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

Интересуетесь передовым инженерным опытом? Присоединяйтесь к нам и помогите нам сделать нашу команду еще лучше!

Первоначально опубликовано на https://engineering.thumbtack.com 18 мая 2018 г.