Comments 34
ваши коллеги такие же профессионалы как вы, вне зависимости от объема личного опыта
Как то не вяжется одно с другим. Профессионализм и определяется количеством личного опыта. Чем он больше, тем человек профессиональнее. С какого момента человек становится профессионалом, это вопрос для дискуссии.
А вообще, Вы как то очень толерантны, а толерантность до хорошего не доводит.
Вот сидит человек, делает какую одну и ту же ошибку раз за разом. Ему раз сказали, два, потом возникает нормальный вопрос, " Эй чувак, ты в танке или просто игнорируешь всех?". И что, надо боятся что он обидится? Думаю не надо.
Надо же с чего-то начинать :). Да, автор немного романтичен и толерантен, но это не самая худшая позиция относительно коде ревью.
Гораздо чаще возникают ситуации когда ревьюер исходя из собственного опыта придирается к человеку с другим опытом.
Ну например Java код, метод возвращает пустой список если параметры метода ни прошли какие то проверки(например). Программист напишет:
return Collections.emptyList(); (Collections.emptyList() возвращает неизменяемый список)
а ревьюер исправит:
return new ArrayList<>();
При этом согласно коду программиста список не предполагается изменять в будущем, но ревьюер исходя из своего личного опыта делает вывод что обязательно найдется кто-нибудь кто попытается засунуть в этот список что-нибудь. Казалось бы оба по своему правы, но в целом это скорее всего придирка.
Ну например Java код, метод возвращает пустой список если параметры метода ни прошли какие то проверки(например). Программист напишет:
return Collections.emptyList(); (Collections.emptyList() возвращает неизменяемый список)
а ревьюер исправит:
return new ArrayList<>();
При этом согласно коду программиста список не предполагается изменять в будущем, но ревьюер исходя из своего личного опыта делает вывод что обязательно найдется кто-нибудь кто попытается засунуть в этот список что-нибудь. Казалось бы оба по своему правы, но в целом это скорее всего придирка.
Вообще, насколько я помню, изменять возвращаемую коллекцию давно считается дурным тоном, кроме исключительных случаев типа ORM, когда обратное указано явно. Где и как с этим бороться — до возврата или после, оборачивать в неизменяемые списки или копировать — вопрос отдельный, но, мне кажется, такое надо явно прописывать в политике кодирования… Тут ещё имеется в наличии некий дефект проектирования самого языка Java, что мутабельность коллекций не отражается ни на уровне языка ни на уровне системы классов и даже, как мне известно (может, когда-нибудь поменяется?), на уровне специальных аннотаций типа
Nullable/NotNull.
Это какой-то очень странный ревьюер. За 15 лет я ни разу не встречался с ситуацией, когда кто-либо предлагал бы заменить в return неизменяемый список на изменяемый — если что-то и предлагалось, то только в обратную сторону (заменять изменяемый на неизменяемый).
Впрочем, с основным посылом (что опыт разных людей может противоречить друг другу) я соглашусь. Но ведь для решения этой проблемы и существуют обсуждения во время code review — и если ни одна из участников не является совсем тупым, то они вполне способны прийти к согласию. А тупым не место в коллективной разработке.
Впрочем, с основным посылом (что опыт разных людей может противоречить друг другу) я соглашусь. Но ведь для решения этой проблемы и существуют обсуждения во время code review — и если ни одна из участников не является совсем тупым, то они вполне способны прийти к согласию. А тупым не место в коллективной разработке.
А у вас проводятся ревью в компании? Просто я работал года 2-3 в компании с ревью, и насмотрелся всякого. Этот случай не самый худший из моей практики. У нас даже велась статистика чей патч заворачивали больше всего раз(по моему лидер был ~30 раз). У меня рекорд 17 раз.
При этом интересно что люди стоящие выше(тех дир и его окружение) заворачивали гораздо реже патчи, чем теже самые лиды. Вот лиды отрывались по полной.
Поэтому я читая эту заметку понимаю что хотел сказать автор и в принципе с ним согласен.
При этом интересно что люди стоящие выше(тех дир и его окружение) заворачивали гораздо реже патчи, чем теже самые лиды. Вот лиды отрывались по полной.
Поэтому я читая эту заметку понимаю что хотел сказать автор и в принципе с ним согласен.
В данное время формальных code review не проводится (маленькая команда + малое количество изменений + постоянные обсуждения до коммитов = проще не заморачиваться. а при желании можно посмотреть историю коммитов и обсудить). Но в других компаниях (да и в этой при взаимодействии с другими командами), разумеется, был опыт честного формального code review. Возможно, конечно, что мне везло на людей, но вопросы и замечания всегда были адекватные и по делу (сам я тоже стараюсь).
Насчёт заворачивания патчей — лично я не вижу в этом ничего плохого, особенно если речь идёт о новичке, или практика code review только начинает вводиться. Да, 17-30 отлупов — это многовато (возможно, слишком торопились?), но 1-2 — вполне нормально, ничего криминального в этом нет.
Насчёт заворачивания патчей — лично я не вижу в этом ничего плохого, особенно если речь идёт о новичке, или практика code review только начинает вводиться. Да, 17-30 отлупов — это многовато (возможно, слишком торопились?), но 1-2 — вполне нормально, ничего криминального в этом нет.
С какого момента человек становится профессионалом, это вопрос для дискуссии.
Формально, человек становится профессионалом с того момента, когда начинает получать деньги за свою работу. Этот термин означает, что человек сделал что-то своей профессией. Например, тот, кто фотографирует для себя не профессионал, даже если он просто изумительный фотограф с известнейшими работами. А тот, кто фотографирует за деньги — профессионал, даже если его фотографии отвратительного качества.
Профессионал != мастер своего дела. Эти термины очень часто путают.
— " Эй чувак, ты в танке или просто игнорируешь всех?".
Обычно если такое повторяется, то тимлид в личной беседе дает указания, если и они игнорируютйса, то на следуюшем performance review ре3ультат будет близок к 0 и HR быстро избавится от неумного человека.
Обычно если такое повторяется, то тимлид в личной беседе дает указания, если и они игнорируютйса, то на следуюшем performance review ре3ультат будет близок к 0 и HR быстро избавится от неумного человека.
а можно ссылку на оригинальную статью?
Зеленое — ссылка на оригинальную статью:
Я с мобильного приложения смотрел, не нашел. Но все-равно спасибо :)
Boomburum friendly reminder :)
Ну и, ИМХО, краткость конечно сестра, но совершенно неочевидно что зеленое имя автора — это ссылка на оригинал.
Ну и, ИМХО, краткость конечно сестра, но совершенно неочевидно что зеленое имя автора — это ссылка на оригинал.
В общем статья права — социальный аспект очень важен, но: code review — это и способ обучения ваших коллег и место для дискуссий.
Ведь дело не только в замере качества кода, но и в профилактике. Мне нужно понять почему программист использовал именно этот вариант решения. Согласитесь, что специалистов «во всем» не бывает и кто может не знать о возможных проблемах производительности или безопасности. А кто то не знает существующих утилит или не понял бизнес требования функции.
Обсуждая код можно не только понять, что подвигло на именно этот вариант исполнения, но и сделать новую запись в списке принятых практик или ввести базовый класс, который позволит избежать недочетов и ошибок в будущем.
Для хорошего программиста возможность учиться и развиваться очень важна. Не получая достаточных отзызвов он не сможет определить свой уровень и направление движения.
Ведь дело не только в замере качества кода, но и в профилактике. Мне нужно понять почему программист использовал именно этот вариант решения. Согласитесь, что специалистов «во всем» не бывает и кто может не знать о возможных проблемах производительности или безопасности. А кто то не знает существующих утилит или не понял бизнес требования функции.
Обсуждая код можно не только понять, что подвигло на именно этот вариант исполнения, но и сделать новую запись в списке принятых практик или ввести базовый класс, который позволит избежать недочетов и ошибок в будущем.
Для хорошего программиста возможность учиться и развиваться очень важна. Не получая достаточных отзызвов он не сможет определить свой уровень и направление движения.
«Качество» кода настолько рассплывчатое понятие, большой простор для конфликтов. Если вы не пишете на С++, очевидные вещи проще проверять автоматическим линтером, помечая их как ошибки ломающие билд. А все причесать сильно проще за один раз, проводя аудит кода раз в один два месяца.
Есть же проблема потока. Программист отправив код на ревью переключается на другую задачу. А мелкими придирками, вы его вынуждаете переключится обратно на старую. Это бессмысленно.
Есть же проблема потока. Программист отправив код на ревью переключается на другую задачу. А мелкими придирками, вы его вынуждаете переключится обратно на старую. Это бессмысленно.
Стало понятно, почему я никогда не любил code review и считал его вредной практикой: у нас его обычно делать не умеют, а code review — такое дело, что лучше его не делать никак, чем делать плохо.
А почему кстати до сих пор нет облачного сервиса для код ревью? Например я написал свой первый код на Haskell и хочу чтобы меня потыкали знатоки, за $ например. Нет такого?
За деньги не знаю, но есть бесплатный codereview.stackexchange.com
Github, gitlab, bitbucket — все они прекрасно выполняют роль тулзов для код-ревью.
Я все-таки не понял опыт автора насчет пункта 2: он добавил этот пункт в причины для чего нужно использовать code review, а с другой стороны говорит что за всю жизнь не чему новому таким образом не научился. Ремарка про противоречие с пунктом что «code review не является способом обучения» дополнительно запутывает.
«Это не место для дискуссий.» — я с этим не согласен, а где же тогда место для дискуссий по коду? Чем вызвано то обсуждать лучшие практики нужно лично, а не в рамках CR, близко к коду?
В статье есть здравое зерно что ревьювер должен быть довольно аккуратным, не вмешиваться например в какие-нибудь стилевые вещи (вроде «я бы по другому написал»), и свои замечания, доносить довольно корректно, а не сверху вниз. В рамках CR на мой взгляд можно спросить почему было выбрано такое архитектурное решение, обсудить варианты. Возможно автор изменений рассматривал и отбросил идеи которые ревьюверу кажутся хорошими, возможно он не догадался и готов будет обдумать такой вариант.
«Это не место для дискуссий.» — я с этим не согласен, а где же тогда место для дискуссий по коду? Чем вызвано то обсуждать лучшие практики нужно лично, а не в рамках CR, близко к коду?
В статье есть здравое зерно что ревьювер должен быть довольно аккуратным, не вмешиваться например в какие-нибудь стилевые вещи (вроде «я бы по другому написал»), и свои замечания, доносить довольно корректно, а не сверху вниз. В рамках CR на мой взгляд можно спросить почему было выбрано такое архитектурное решение, обсудить варианты. Возможно автор изменений рассматривал и отбросил идеи которые ревьюверу кажутся хорошими, возможно он не догадался и готов будет обдумать такой вариант.
Согласен, что как раз таки CR место для дискуссий, более того таким образом сохраняется история и через некоторое время можно будет понять почему сделано именно так. А если что-то кажется достаточно спорным чтобы это обсуждать, то с большой вероятностью и позже возникнет вопрос: «почему сделано так?»
Я, наверное, сейчас напишу банальные вещи, но всё же, я думаю, что всю суть статьи можно выразить так:
Со стороны того, кто проверяет:
— Нужно быть сдержанным и вежливым
— Не нужно никого оскорблять, унижать, переходить на личности, самоутверждаться
— Нужно объяснять, почему этот код плохой и как сделать его лучше
Со стороны того, чей код проверяют:
— Нужно уметь адекватно реагировать на критику своего кода, не воспринимать критику своей работы как личное оскорбление
— Нужно быть терпеливым и не нервничать если просмотр кода затягивается
— Воспринимать code review как возможность для личного профессионального роста и накопления опыта
Со стороны того, кто проверяет:
— Нужно быть сдержанным и вежливым
— Не нужно никого оскорблять, унижать, переходить на личности, самоутверждаться
— Нужно объяснять, почему этот код плохой и как сделать его лучше
Со стороны того, чей код проверяют:
— Нужно уметь адекватно реагировать на критику своего кода, не воспринимать критику своей работы как личное оскорбление
— Нужно быть терпеливым и не нервничать если просмотр кода затягивается
— Воспринимать code review как возможность для личного профессионального роста и накопления опыта
Ну вот вроде все правильно, но уже не раз в своей практике сталкивался с тем, что человек, код которого проверяется, просто морально не готов слушать критику и тем более смириться с отказом в принятии пачта, даже если он в проекте «15 минут». бОльшая часть замечаний проход под лозунгом «Окей, я учту в следующих задачах». Тут еще, как мне кажется, важна разница в возрасте — старшие товарищи по цеху хуже воспринимают критику. Доходило до того, что мне тыкали поездкой на "… всесоюзную олимпиаду по программированию для школьников, аж в Москву ездил". А ты, мол, тогда еще пешком под стол ходил.
фуу, накипело :)
посоветуете что-нибудь?
фуу, накипело :)
посоветуете что-нибудь?
Есть такая практика, Mob Programming. Это как программирование в паре, но всей командой, полный фокус задача за задачей. Завидую им — им Code Review делать необязательно. Кто-нибудь применял такое?
Only those users with full accounts are able to leave comments. Log in, please.
Перевод: Этика Code Review