Культура проверки кода: почему она вам нужна

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

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

Независимо от того, какой вариант вы выберете — просто применить все изменения или отталкиваться с длинными объяснениями, почему вы не согласны с тем или иным — вы, скорее всего, в конечном итоге почувствуете себя взволнованным, измотанным и разочарованным из-за времени, потраченного на несущественные вещи. И стоило ли оно того? Этот вопрос будет возникать у вас в голове каждый раз, когда вы будете иметь дело с такими ситуациями.

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

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

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

Будучи разработчиком iOS, я буду использовать Swift в своих примерах кода, но в целом эта тема важна для любого разработчика, независимо от платформы или языка, которые вы используете ежедневно.

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

Урок усвоен

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

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

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

Много лет назад, когда я искал новую работу, мне бросили вызов с насмешливым запросом на включение, который мне нужно было просмотреть. Я не обдумал все как следует в тот раз, потому что был взволнован тем, что получил легкую задачу вместо очередной пытки Leetcode. Конечно, это было не так уж и легко. Это был тест на культуру запросов на включение, который я провалил с треском.

Я быстро просмотрел PR и оставил несколько в основном бессмысленных комментариев, например:

 import UIKit import AVFoundation // It would be nice to list your imports alphabetically, so that it's easier to read.

Или еще:

 func someMethod() { // about 30 lines of code } // What do you think about splitting this method in half?

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

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

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

С тех пор я понял, что мой подход к проверке кода далек от совершенства и в нем не хватает многих важных вещей.

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

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

Что такое придирчивый комментарий?

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

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

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

 // Original guard let newValue, newValue != oldValue else { return } // Change request guard let newValue, newValue != oldValue else { return }

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

 // Original func handleErrorIfNeeded(_ error: SomeError) { if error == .failedRequest { return } if error == .validationFailed { state.errorMessage = “Data input is incorrect” return } // … other cases } // Change request func handleErrorIfNeeded(_ error: SomeError) { guard error != .failedRequest else { return } guard error != .validationFailed { state.errorMessage = “Data input is incorrect” return } // … other cases }

Во многих логических ситуациях guard и if в Swift могут использоваться взаимозаменяемо. Однако я бы сказал, что у них разная языковая семантика. guard , как следует из названия, хочет «защитить» ваш поток кода от нежелательного результата, тогда как if нейтрален по своей природе и допускает вероятность 50-50% возникновения различных путей кода. Хотя технически оба варианта не являются неправильными и прекрасно читаются.

И последнее:

 // Original, the constant value is not being repeated anywhere else func calculateDuration(endDuration: Duration) -> Duration { let startDuration = Duration.milliseconds(130) // … calculation return finalDuration } // Change request func calculateDuration(endDuration: Duration) -> Duration { let startDuration = Duration.milliseconds(Constant.startDuration) // … calculation return finalDuration } enum Constant { static let startDuration = Double(130) }

Можно спорить о необходимости извлечения любой константы в отдельное пространство имен, если она используется только один раз и когда это не магическая константа. В первом варианте вы четко знаете, что такое это число 130. Однако кто-то скажет вам извлекать любую константу вроде этой, несмотря ни на что.

Все приведенные выше примеры — это мелочные изменения. Оба варианта вполне хороши, читаются любым и не нарушают ваш код. Это просто альтернативы с одинаковым весом в использовании.

Итак, что вы хотите сделать с такими комментариями? С точки зрения рецензента, некоторые скажут что-то вроде этого:

Ну, это всего лишь предложение. Его не обязательно применять.

Это верно, но в то же время важно дать знать об этом автору, например:

Это всего лишь предложение, можете смело его отвергнуть, но что вы думаете о таком подходе: …?

Однако я считаю, что придирки лучше вообще не публиковать . Почему?

Допустим, вы отправили придирчивый комментарий, даже подчеркнули, что это предложение с низким приоритетом. Что происходит дальше?

• 1-й вариант: автор применяет изменение. Что это на самом деле означает за кулисами?
  • Им необходимо сохранить текущие изменения, которые они вносят.
  • Далее им нужно переключиться на отдел связей с общественностью.
  • Им необходимо применить это изменение.
  • Они могут разрешать конфликты.
  • Им нужно продвигать изменения.
  • Затем он запустит еще один раунд тестов CI из-за нового переданного кода.
  • Им необходимо ответить на комментарий текстом или эмодзи.

С одной стороны, это очень простой набор задач, которые мы выполняем каждый день, как разработчики. Но представьте себе, это не один придирчивый комментарий, а 10. Или у вас одновременно открыто 5 запросов на включение изменений с придирками разного количества. Стоило ли это вашего времени и хлопот? Когда мы знаем, что такое придирка, ответ — нет. Я бы предпочел быстрее доставить свои изменения и сэкономить время.

• 2-й вариант: автор не применяет изменение.

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

    
    

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

  
  

  • Вместо этого, как автор, вы, вероятно, углубитесь в объяснение деталей того, почему вы не будете применять изменения. Например, ваше видение на guard против let , как указано выше.

    
    

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

    
    

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

  
  

Я, как рецензент, стараюсь отфильтровывать подобные придирки в обзорах кода еще до публикации чего-либо, задавая себе следующие вопросы:

Действительно ли это важно для изменений?

Есть ли объективные причины для этого? Если да, то назовите их.

Хочу ли я опубликовать это замечание только потому, что я бы сделал это по-другому? Или есть реальная обеспокоенность?

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

Но почему такие комментарии появляются? Обычно это признак того, что у рецензентов нет четкого понимания того, на чем следует сосредоточиться или что искать в pull-реквестах. В следующем разделе мы обсудим именно это.

Как создать культуру обзора кода

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

1. Для всех: Определите набор сегментов или категорий для комментариев по обзору кода и назначьте им разные уровни серьезности. Всегда показывайте примеры коллегам, чтобы вы все были на одной странице. Пример:

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

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

   
   

3. Автор: Помогите своим коллегам. Если вы знаете, что некоторые части могут быть неясны из-за отсутствия контекста или потому, что ваш PR — это только одна часть в серии, которая будет продолжена, просто оставьте комментарий с объяснением вашего собственного pull request заранее. Это сэкономит время для обеих сторон!

   
   

4. Рецензент: перечитайте свои комментарии перед публикацией. Поставьте себя на место другой стороны. Все ли понятно? Постарайтесь придерживаться следующей структуры:

   1. Выскажите свое беспокойство.
   2. Почему это важно.
   3. Что вы предлагаете делать вместо этого?

   
   

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

   
   

6. Для всех: Как воспринимается ваш комментарий? Звучит ли он дружелюбно? Этот пункт довольно похож на предыдущий, но идея в том, чтобы сохранить здоровый, дружелюбный подход в письменной форме. Например:

   
   

   Эту логику необходимо выделить в отдельный метод.

   
   

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

   
   

   Что вы думаете о выделении этой логики в отдельный метод?

   
   

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

   
   

8. Рецензент: Если в изменениях кода все хорошо, не стесняйтесь публиковать похвалу. Одобрение в Instagram без знаков — это не такое уж хорошее поведение и оставляет много вопросов. Опубликуйте хвалебные отзывы с указанием именно тех вещей, которые вы нашли хорошими в работе вашего коллеги. Простое «LGTM» слишком общее и может быть воспринято как пренебрежение.

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

Заключение

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

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

Удачи вам всем и дружелюбных проверок кода!