Недавно мне на глаза попалась небольшая, но очень емкая статья одного разработчика, в которой он рассматривает code reiew с довольно неожиданной стороны. Большинство материалов, посвященных code review, рассказывают о технических вопросах: какими утилитами пользоваться, как интегрировать code review в процесс continuous integration и прочие технические моменты. Автор же рассматривает code review как социальное взаимодействие. И, на мой взгляд, об этом стоит почитать. Под катом — перевод.Так же как для многих других вещей в нашей жизни, знание зачем мы что-то делаем позволяет делать это действительно хорошо и не поддаваться желанию делать это тогда, когда оно не нужно. Начнем с того, что перечислим чем code review не является:
- Это не полчаса в день приятного времяпровождения когда вы кричите на своих коллег и тыкаете пальцем в их ошибки. Если вам нравится делать code review чтобы находить чужие ошибки или отсутствие хороших практик в коде, то у меня для вас плохие новости — похоже, это не лучшее для вас занятие.
- Это не способ обучения ваших коллег. Если вы считаете, что команда должна придерживаться определенных практик при разработке, то необходимо отдельно выделить время и согласовать эти практики с разработчиками лично.
- Это не место для дискуссий. Комментарии в code review должны быть максимально короткими и по делу. Никаких попыток отстаивать свою точку зрения или защищать свою позицию.
При таком раскладе — зачем тогда нужно code review? Основываясь на моей практике, я могу выделить четыре причины:
- Это способ разделить ответственность. Когда мы делаем code review то мы, образно выражаясь, «помогает тянуть лямку». Мы отказываемся от нашего обычного «меня это не @#$%!@#, не мой код» в пользу «ну что ж, будем валить этого дракона вместе».
- Это замечательный способ учиться друг у друга. На первый взгляд, этот пункт противоречит второму пункту предыдущего списка, но поверьте мне на слово. За свою карьеру я узнал несравнимо больше нового от дружеских, неагрессивных обсуждений с коллегами, нежели ото всех этих агрессивных придирок к моему коду от людей, которые думают что знают все лучше всех (или по крайней мере лучше чем я). Более того — мне так и не удалось научиться извлекать пользу из таких комментариев, только острое чувство ненависти (к комментариям, не к людям). Вне зависимости от того, насколько вы лучше, другие люди заслуживают уважительного к себе отношения. Хотя бы потому, что вы сами когда-то были таким.
- Это способ находить баги и явные ошибки. По-хорошему, это — основная причина, чтобы делать code review. Просматривая чужой код, фокусируйтесь на поиске ошибок и багов, которые автор кода мог проглядеть — и предлагайте простые способы их исправить. Иногда вы даже будете встречать одну и ту же ошибку, повторенную несколько раз. Боритесь с искушением писать комментарии вроде «парень, зачем ты опять делаешь ЭТО?!?». Будьте профессиональны. И если вы считаете, что какие-то моменты в коде ваших коллег нужно обсудить — сделайте это лично, вежливо и без «наездов».
- Это способ улучшить качество кода. Как я уже писал, лучшие практики нужно обсуждать с коллегами лично. Но если вы нашли в коде какое-то место, которое, как вы считаете, может быть легко улучшено — будет хорошей идеей вежливо предложить это в рамках code review.
Подводя итог: всегда нужно помнить, что ваши коллеги такие же профессионалы, как вы, вне зависимости от объема личного опыта. Но, что гораздо важнее — они живые люди. А люди лучше и продуктивнее работают и более счастливы, когда их понимают и уважают. И code review может в этом помочь — если правильно его использовать.
Комментарии (32)
t1gor
23.11.2015 10:24а можно ссылку на оригинальную статью?
kronoskib
23.11.2015 11:00+4Зеленое — ссылка на оригинальную статью:
t1gor
23.11.2015 11:13Я с мобильного приложения смотрел, не нашел. Но все-равно спасибо :)
eyeofhell
23.11.2015 11:24+4Boomburum friendly reminder :)
Ну и, ИМХО, краткость конечно сестра, но совершенно неочевидно что зеленое имя автора — это ссылка на оригинал.
Boomburum
23.11.2015 11:45+2Передал информацию куда надо :)
eugenius_nsk
23.11.2015 17:07-2В итоге «откуда надо» прилетит НЛО и одним недовольным станет меньше? ;-)
XaBoK
23.11.2015 11:02+5В общем статья права — социальный аспект очень важен, но: code review — это и способ обучения ваших коллег и место для дискуссий.
Ведь дело не только в замере качества кода, но и в профилактике. Мне нужно понять почему программист использовал именно этот вариант решения. Согласитесь, что специалистов «во всем» не бывает и кто может не знать о возможных проблемах производительности или безопасности. А кто то не знает существующих утилит или не понял бизнес требования функции.
Обсуждая код можно не только понять, что подвигло на именно этот вариант исполнения, но и сделать новую запись в списке принятых практик или ввести базовый класс, который позволит избежать недочетов и ошибок в будущем.
Для хорошего программиста возможность учиться и развиваться очень важна. Не получая достаточных отзызвов он не сможет определить свой уровень и направление движения.eyeofhell
23.11.2015 11:27Соглашусь с автором что такие вещи лучше переносить на очные обсуждения. А то засоряется история, возникают сложности коммуникации, «лесенки» из вопросов и ответов на пол экрана :)
Dimusik
23.11.2015 11:33+3Все правильно, только профилактика не должна выражаться в стиле: «ты что, тупая обезьяна?!».
kmmbvnr
23.11.2015 11:42+1«Качество» кода настолько рассплывчатое понятие, большой простор для конфликтов. Если вы не пишете на С++, очевидные вещи проще проверять автоматическим линтером, помечая их как ошибки ломающие билд. А все причесать сильно проще за один раз, проводя аудит кода раз в один два месяца.
Есть же проблема потока. Программист отправив код на ревью переключается на другую задачу. А мелкими придирками, вы его вынуждаете переключится обратно на старую. Это бессмысленно.
Flammar
23.11.2015 11:53+1Стало понятно, почему я никогда не любил code review и считал его вредной практикой: у нас его обычно делать не умеют, а code review — такое дело, что лучше его не делать никак, чем делать плохо.
rax
23.11.2015 12:34А почему кстати до сих пор нет облачного сервиса для код ревью? Например я написал свой первый код на Haskell и хочу чтобы меня потыкали знатоки, за $ например. Нет такого?
return_true
23.11.2015 12:44+1За деньги не знаю, но есть бесплатный codereview.stackexchange.com
rax
23.11.2015 13:09Там как-то больше спрашивают почему этот сниппет не работает, при этом не видно архитектуры, файловой структуры итд. К тому же нет никакой гарантии, что найдется альтруист копающийся в твоем г)
orcy
23.11.2015 14:05Я все-таки не понял опыт автора насчет пункта 2: он добавил этот пункт в причины для чего нужно использовать code review, а с другой стороны говорит что за всю жизнь не чему новому таким образом не научился. Ремарка про противоречие с пунктом что «code review не является способом обучения» дополнительно запутывает.
«Это не место для дискуссий.» — я с этим не согласен, а где же тогда место для дискуссий по коду? Чем вызвано то обсуждать лучшие практики нужно лично, а не в рамках CR, близко к коду?
В статье есть здравое зерно что ревьювер должен быть довольно аккуратным, не вмешиваться например в какие-нибудь стилевые вещи (вроде «я бы по другому написал»), и свои замечания, доносить довольно корректно, а не сверху вниз. В рамках CR на мой взгляд можно спросить почему было выбрано такое архитектурное решение, обсудить варианты. Возможно автор изменений рассматривал и отбросил идеи которые ревьюверу кажутся хорошими, возможно он не догадался и готов будет обдумать такой вариант.McElroy
23.11.2015 23:27Согласен, что как раз таки CR место для дискуссий, более того таким образом сохраняется история и через некоторое время можно будет понять почему сделано именно так. А если что-то кажется достаточно спорным чтобы это обсуждать, то с большой вероятностью и позже возникнет вопрос: «почему сделано так?»
iroln
23.11.2015 16:00+8Я, наверное, сейчас напишу банальные вещи, но всё же, я думаю, что всю суть статьи можно выразить так:
Со стороны того, кто проверяет:
— Нужно быть сдержанным и вежливым
— Не нужно никого оскорблять, унижать, переходить на личности, самоутверждаться
— Нужно объяснять, почему этот код плохой и как сделать его лучше
Со стороны того, чей код проверяют:
— Нужно уметь адекватно реагировать на критику своего кода, не воспринимать критику своей работы как личное оскорбление
— Нужно быть терпеливым и не нервничать если просмотр кода затягивается
— Воспринимать code review как возможность для личного профессионального роста и накопления опыта
avesus
23.11.2015 21:00Есть такая практика, Mob Programming. Это как программирование в паре, но всей командой, полный фокус задача за задачей. Завидую им — им Code Review делать необязательно. Кто-нибудь применял такое?
boodda
Как то не вяжется одно с другим. Профессионализм и определяется количеством личного опыта. Чем он больше, тем человек профессиональнее. С какого момента человек становится профессионалом, это вопрос для дискуссии.
А вообще, Вы как то очень толерантны, а толерантность до хорошего не доводит.
Вот сидит человек, делает какую одну и ту же ошибку раз за разом. Ему раз сказали, два, потом возникает нормальный вопрос, " Эй чувак, ты в танке или просто игнорируешь всех?". И что, надо боятся что он обидится? Думаю не надо.
kronoskib
Надо же с чего-то начинать :). Да, автор немного романтичен и толерантен, но это не самая худшая позиция относительно коде ревью.
vayho
Гораздо чаще возникают ситуации когда ревьюер исходя из собственного опыта придирается к человеку с другим опытом.
Ну например Java код, метод возвращает пустой список если параметры метода ни прошли какие то проверки(например). Программист напишет:
return Collections.emptyList(); (Collections.emptyList() возвращает неизменяемый список)
а ревьюер исправит:
return new ArrayList<>();
При этом согласно коду программиста список не предполагается изменять в будущем, но ревьюер исходя из своего личного опыта делает вывод что обязательно найдется кто-нибудь кто попытается засунуть в этот список что-нибудь. Казалось бы оба по своему правы, но в целом это скорее всего придирка.
Flammar
Вообще, насколько я помню, изменять возвращаемую коллекцию давно считается дурным тоном, кроме исключительных случаев типа ORM, когда обратное указано явно. Где и как с этим бороться — до возврата или после, оборачивать в неизменяемые списки или копировать — вопрос отдельный, но, мне кажется, такое надо явно прописывать в политике кодирования… Тут ещё имеется в наличии некий дефект проектирования самого языка Java, что мутабельность коллекций не отражается ни на уровне языка ни на уровне системы классов и даже, как мне известно (может, когда-нибудь поменяется?), на уровне специальных аннотаций типа
Nullable/NotNull.eugenius_nsk
Это какой-то очень странный ревьюер. За 15 лет я ни разу не встречался с ситуацией, когда кто-либо предлагал бы заменить в return неизменяемый список на изменяемый — если что-то и предлагалось, то только в обратную сторону (заменять изменяемый на неизменяемый).
Впрочем, с основным посылом (что опыт разных людей может противоречить друг другу) я соглашусь. Но ведь для решения этой проблемы и существуют обсуждения во время code review — и если ни одна из участников не является совсем тупым, то они вполне способны прийти к согласию. А тупым не место в коллективной разработке.
vayho
А у вас проводятся ревью в компании? Просто я работал года 2-3 в компании с ревью, и насмотрелся всякого. Этот случай не самый худший из моей практики. У нас даже велась статистика чей патч заворачивали больше всего раз(по моему лидер был ~30 раз). У меня рекорд 17 раз.
При этом интересно что люди стоящие выше(тех дир и его окружение) заворачивали гораздо реже патчи, чем теже самые лиды. Вот лиды отрывались по полной.
Поэтому я читая эту заметку понимаю что хотел сказать автор и в принципе с ним согласен.
eugenius_nsk
В данное время формальных code review не проводится (маленькая команда + малое количество изменений + постоянные обсуждения до коммитов = проще не заморачиваться. а при желании можно посмотреть историю коммитов и обсудить). Но в других компаниях (да и в этой при взаимодействии с другими командами), разумеется, был опыт честного формального code review. Возможно, конечно, что мне везло на людей, но вопросы и замечания всегда были адекватные и по делу (сам я тоже стараюсь).
Насчёт заворачивания патчей — лично я не вижу в этом ничего плохого, особенно если речь идёт о новичке, или практика code review только начинает вводиться. Да, 17-30 отлупов — это многовато (возможно, слишком торопились?), но 1-2 — вполне нормально, ничего криминального в этом нет.
Dolios
Формально, человек становится профессионалом с того момента, когда начинает получать деньги за свою работу. Этот термин означает, что человек сделал что-то своей профессией. Например, тот, кто фотографирует для себя не профессионал, даже если он просто изумительный фотограф с известнейшими работами. А тот, кто фотографирует за деньги — профессионал, даже если его фотографии отвратительного качества.
Профессионал != мастер своего дела. Эти термины очень часто путают.
boodda
В таком ключе, да вы абсолютно правы.В моём комментарии надо заменить слово профессионализм на мастерство. Но суть моего комментария это, вообщем то, не меняет.
BalinTomsk
— " Эй чувак, ты в танке или просто игнорируешь всех?".
Обычно если такое повторяется, то тимлид в личной беседе дает указания, если и они игнорируютйса, то на следуюшем performance review ре3ультат будет близок к 0 и HR быстро избавится от неумного человека.