Компания
202,10
рейтинг
23 ноября 2015 в 09:39

Разработка → Перевод: Этика Code Review перевод

Недавно мне на глаза попалась небольшая, но очень емкая статья одного разработчика, в которой он рассматривает code reiew с довольно неожиданной стороны. Большинство материалов, посвященных code review, рассказывают о технических вопросах: какими утилитами пользоваться, как интегрировать code review в процесс continuous integration и прочие технические моменты. Автор же рассматривает code review как социальное взаимодействие. И, на мой взгляд, об этом стоит почитать. Под катом — перевод.


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

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


При таком раскладе — зачем тогда нужно code review? Основываясь на моей практике, я могу выделить четыре причины:

  1. Это способ разделить ответственность. Когда мы делаем code review то мы, образно выражаясь, «помогает тянуть лямку». Мы отказываемся от нашего обычного «меня это не @#$%!@#, не мой код» в пользу «ну что ж, будем валить этого дракона вместе».
  2. Это замечательный способ учиться друг у друга. На первый взгляд, этот пункт противоречит второму пункту предыдущего списка, но поверьте мне на слово. За свою карьеру я узнал несравнимо больше нового от дружеских, неагрессивных обсуждений с коллегами, нежели ото всех этих агрессивных придирок к моему коду от людей, которые думают что знают все лучше всех (или по крайней мере лучше чем я). Более того — мне так и не удалось научиться извлекать пользу из таких комментариев, только острое чувство ненависти (к комментариям, не к людям). Вне зависимости от того, насколько вы лучше, другие люди заслуживают уважительного к себе отношения. Хотя бы потому, что вы сами когда-то были таким.
  3. Это способ находить баги и явные ошибки. По-хорошему, это — основная причина, чтобы делать code review. Просматривая чужой код, фокусируйтесь на поиске ошибок и багов, которые автор кода мог проглядеть — и предлагайте простые способы их исправить. Иногда вы даже будете встречать одну и ту же ошибку, повторенную несколько раз. Боритесь с искушением писать комментарии вроде «парень, зачем ты опять делаешь ЭТО?!?». Будьте профессиональны. И если вы считаете, что какие-то моменты в коде ваших коллег нужно обсудить — сделайте это лично, вежливо и без «наездов».
  4. Это способ улучшить качество кода. Как я уже писал, лучшие практики нужно обсуждать с коллегами лично. Но если вы нашли в коде какое-то место, которое, как вы считаете, может быть легко улучшено — будет хорошей идеей вежливо предложить это в рамках code review.


Подводя итог: всегда нужно помнить, что ваши коллеги такие же профессионалы, как вы, вне зависимости от объема личного опыта. Но, что гораздо важнее — они живые люди. А люди лучше и продуктивнее работают и более счастливы, когда их понимают и уважают. И code review может в этом помочь — если правильно его использовать.
Автор: @kronoskib Marco Troisi

Похожие публикации

Комментарии (34)

  • +3
    ваши коллеги такие же профессионалы как вы, вне зависимости от объема личного опыта

    Как то не вяжется одно с другим. Профессионализм и определяется количеством личного опыта. Чем он больше, тем человек профессиональнее. С какого момента человек становится профессионалом, это вопрос для дискуссии.

    А вообще, Вы как то очень толерантны, а толерантность до хорошего не доводит.
    Вот сидит человек, делает какую одну и ту же ошибку раз за разом. Ему раз сказали, два, потом возникает нормальный вопрос, " Эй чувак, ты в танке или просто игнорируешь всех?". И что, надо боятся что он обидится? Думаю не надо.
    • +6
      Надо же с чего-то начинать :). Да, автор немного романтичен и толерантен, но это не самая худшая позиция относительно коде ревью.
    • +2
      Гораздо чаще возникают ситуации когда ревьюер исходя из собственного опыта придирается к человеку с другим опытом.
      Ну например Java код, метод возвращает пустой список если параметры метода ни прошли какие то проверки(например). Программист напишет:
      return Collections.emptyList(); (Collections.emptyList() возвращает неизменяемый список)
      а ревьюер исправит:
      return new ArrayList<>();
      При этом согласно коду программиста список не предполагается изменять в будущем, но ревьюер исходя из своего личного опыта делает вывод что обязательно найдется кто-нибудь кто попытается засунуть в этот список что-нибудь. Казалось бы оба по своему правы, но в целом это скорее всего придирка.
      • +2
        Вообще, насколько я помню, изменять возвращаемую коллекцию давно считается дурным тоном, кроме исключительных случаев типа ORM, когда обратное указано явно. Где и как с этим бороться — до возврата или после, оборачивать в неизменяемые списки или копировать — вопрос отдельный, но, мне кажется, такое надо явно прописывать в политике кодирования… Тут ещё имеется в наличии некий дефект проектирования самого языка Java, что мутабельность коллекций не отражается ни на уровне языка ни на уровне системы классов и даже, как мне известно (может, когда-нибудь поменяется?), на уровне специальных аннотаций типа Nullable/NotNull.
      • 0
        Это какой-то очень странный ревьюер. За 15 лет я ни разу не встречался с ситуацией, когда кто-либо предлагал бы заменить в return неизменяемый список на изменяемый — если что-то и предлагалось, то только в обратную сторону (заменять изменяемый на неизменяемый).

        Впрочем, с основным посылом (что опыт разных людей может противоречить друг другу) я соглашусь. Но ведь для решения этой проблемы и существуют обсуждения во время code review — и если ни одна из участников не является совсем тупым, то они вполне способны прийти к согласию. А тупым не место в коллективной разработке.
        • 0
          А у вас проводятся ревью в компании? Просто я работал года 2-3 в компании с ревью, и насмотрелся всякого. Этот случай не самый худший из моей практики. У нас даже велась статистика чей патч заворачивали больше всего раз(по моему лидер был ~30 раз). У меня рекорд 17 раз.
          При этом интересно что люди стоящие выше(тех дир и его окружение) заворачивали гораздо реже патчи, чем теже самые лиды. Вот лиды отрывались по полной.
          Поэтому я читая эту заметку понимаю что хотел сказать автор и в принципе с ним согласен.
          • +1
            В данное время формальных code review не проводится (маленькая команда + малое количество изменений + постоянные обсуждения до коммитов = проще не заморачиваться. а при желании можно посмотреть историю коммитов и обсудить). Но в других компаниях (да и в этой при взаимодействии с другими командами), разумеется, был опыт честного формального code review. Возможно, конечно, что мне везло на людей, но вопросы и замечания всегда были адекватные и по делу (сам я тоже стараюсь).

            Насчёт заворачивания патчей — лично я не вижу в этом ничего плохого, особенно если речь идёт о новичке, или практика code review только начинает вводиться. Да, 17-30 отлупов — это многовато (возможно, слишком торопились?), но 1-2 — вполне нормально, ничего криминального в этом нет.
    • +7
      С какого момента человек становится профессионалом, это вопрос для дискуссии.

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

      Профессионал != мастер своего дела. Эти термины очень часто путают.
      • +1
        В таком ключе, да вы абсолютно правы.В моём комментарии надо заменить слово профессионализм на мастерство. Но суть моего комментария это, вообщем то, не меняет.
    • 0
      — " Эй чувак, ты в танке или просто игнорируешь всех?".

      Обычно если такое повторяется, то тимлид в личной беседе дает указания, если и они игнорируютйса, то на следуюшем performance review ре3ультат будет близок к 0 и HR быстро избавится от неумного человека.
  • 0
    а можно ссылку на оригинальную статью?
    • +1
      • 0
        Спасибо!
    • +4
      Зеленое — ссылка на оригинальную статью:

      • 0
        Я с мобильного приложения смотрел, не нашел. Но все-равно спасибо :)
        • +4
          Boomburum friendly reminder :)

          Ну и, ИМХО, краткость конечно сестра, но совершенно неочевидно что зеленое имя автора — это ссылка на оригинал.
          • +2
            Передал информацию куда надо :)
            • –2
              В итоге «откуда надо» прилетит НЛО и одним недовольным станет меньше? ;-)
  • +5
    В общем статья права — социальный аспект очень важен, но: code review — это и способ обучения ваших коллег и место для дискуссий.
    Ведь дело не только в замере качества кода, но и в профилактике. Мне нужно понять почему программист использовал именно этот вариант решения. Согласитесь, что специалистов «во всем» не бывает и кто может не знать о возможных проблемах производительности или безопасности. А кто то не знает существующих утилит или не понял бизнес требования функции.

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

    Для хорошего программиста возможность учиться и развиваться очень важна. Не получая достаточных отзызвов он не сможет определить свой уровень и направление движения.
    • 0
      Соглашусь с автором что такие вещи лучше переносить на очные обсуждения. А то засоряется история, возникают сложности коммуникации, «лесенки» из вопросов и ответов на пол экрана :)
    • +3
      Все правильно, только профилактика не должна выражаться в стиле: «ты что, тупая обезьяна?!».
  • +1
    «Качество» кода настолько рассплывчатое понятие, большой простор для конфликтов. Если вы не пишете на С++, очевидные вещи проще проверять автоматическим линтером, помечая их как ошибки ломающие билд. А все причесать сильно проще за один раз, проводя аудит кода раз в один два месяца.

    Есть же проблема потока. Программист отправив код на ревью переключается на другую задачу. А мелкими придирками, вы его вынуждаете переключится обратно на старую. Это бессмысленно.
  • +1
    Стало понятно, почему я никогда не любил code review и считал его вредной практикой: у нас его обычно делать не умеют, а code review — такое дело, что лучше его не делать никак, чем делать плохо.
  • 0
    А почему кстати до сих пор нет облачного сервиса для код ревью? Например я написал свой первый код на Haskell и хочу чтобы меня потыкали знатоки, за $ например. Нет такого?
    • +1
      За деньги не знаю, но есть бесплатный codereview.stackexchange.com
      • 0
        Там как-то больше спрашивают почему этот сниппет не работает, при этом не видно архитектуры, файловой структуры итд. К тому же нет никакой гарантии, что найдется альтруист копающийся в твоем г)
    • +1
      Github, gitlab, bitbucket — все они прекрасно выполняют роль тулзов для код-ревью.
      • 0
        Тулза не сервис, у вас никогда не возникала потребности того, о чем я написал?
  • 0
    Я все-таки не понял опыт автора насчет пункта 2: он добавил этот пункт в причины для чего нужно использовать code review, а с другой стороны говорит что за всю жизнь не чему новому таким образом не научился. Ремарка про противоречие с пунктом что «code review не является способом обучения» дополнительно запутывает.

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

    В статье есть здравое зерно что ревьювер должен быть довольно аккуратным, не вмешиваться например в какие-нибудь стилевые вещи (вроде «я бы по другому написал»), и свои замечания, доносить довольно корректно, а не сверху вниз. В рамках CR на мой взгляд можно спросить почему было выбрано такое архитектурное решение, обсудить варианты. Возможно автор изменений рассматривал и отбросил идеи которые ревьюверу кажутся хорошими, возможно он не догадался и готов будет обдумать такой вариант.
    • 0
      Согласен, что как раз таки CR место для дискуссий, более того таким образом сохраняется история и через некоторое время можно будет понять почему сделано именно так. А если что-то кажется достаточно спорным чтобы это обсуждать, то с большой вероятностью и позже возникнет вопрос: «почему сделано так?»
  • +8
    Я, наверное, сейчас напишу банальные вещи, но всё же, я думаю, что всю суть статьи можно выразить так:

    Со стороны того, кто проверяет:
    — Нужно быть сдержанным и вежливым
    — Не нужно никого оскорблять, унижать, переходить на личности, самоутверждаться
    — Нужно объяснять, почему этот код плохой и как сделать его лучше

    Со стороны того, чей код проверяют:
    — Нужно уметь адекватно реагировать на критику своего кода, не воспринимать критику своей работы как личное оскорбление
    — Нужно быть терпеливым и не нервничать если просмотр кода затягивается
    — Воспринимать code review как возможность для личного профессионального роста и накопления опыта
    • 0
      Ну вот вроде все правильно, но уже не раз в своей практике сталкивался с тем, что человек, код которого проверяется, просто морально не готов слушать критику и тем более смириться с отказом в принятии пачта, даже если он в проекте «15 минут». бОльшая часть замечаний проход под лозунгом «Окей, я учту в следующих задачах». Тут еще, как мне кажется, важна разница в возрасте — старшие товарищи по цеху хуже воспринимают критику. Доходило до того, что мне тыкали поездкой на "… всесоюзную олимпиаду по программированию для школьников, аж в Москву ездил". А ты, мол, тогда еще пешком под стол ходил.

      фуу, накипело :)

      посоветуете что-нибудь?
      • +1
        Критику можно и нужно разбавить. Сказать «ох, как классно сделано, не знал про такое», а потом у кофе машины обсудить решение. Ну, мало ли способов показать, что ты не только критикуешь, но и внимательно относишься к коду.
  • 0
    Есть такая практика, Mob Programming. Это как программирование в паре, но всей командой, полный фокус задача за задачей. Завидую им — им Code Review делать необязательно. Кто-нибудь применял такое?

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

Самое читаемое Разработка