Перевод: Этика Code Review

http://www.marcotroisi.com/the-ethics-of-code-reviews/
  • Перевод
Недавно мне на глаза попалась небольшая, но очень емкая статья одного разработчика, в которой он рассматривает 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 может в этом помочь — если правильно его использовать.
Метки:
Voximplant 206,46
Облачная телеком-платформа
Поделиться публикацией
Похожие публикации
Комментарии 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
              а можно ссылку на оригинальную статью?
              • +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 делать необязательно. Кто-нибудь применял такое?

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

                                Самое читаемое