Хабр Курсы для всех
РЕКЛАМА
Большая витрина: от крупнейших школ до частных авторов. Сравнивайте по цене, длительности, формату и выбирайте самый подходящий курс!
Хабр доступен 24/7 благодаря поддержке друзей
Комментарии 15
Не умаляя ценности статьи, от себя добавлю — советы по поводу корректного общения, например использование уточняющих вопросов вместо явных указаний на ошибки или похвалы удачных мест, они же напрямую с реальной жизнью коррелируют. Т.е. не только при проведении code review стоит вести себя уважительно и культурно по отношению к коллегам, это помогает поддерживать здорововые дружеские отношения в коллективе.
Напомнило: habr.com/ru/post/340550
Спасибо за статью. Мне кажеться, что многие разработчики недооценивают важность рекомендация №5. Я бы еще рекомендовал быть готовым выслушать идеи автора кода перед тем как разносить в пух и прах его код. Очень часто бывает, что автор предусмотрел случаи, про которые ревьювер не подумал.
Я всегда воспринимал эту фразу как «Господи, ну давайте уже избавимся от ...». Т.е. «please» в данном случае скорее выражает раздражение, чем вежливую просьбу.
Can we please get rid of...
для вида вежливости добавляя «please»
Я всегда воспринимал эту фразу как «Господи, ну давайте уже избавимся от ...». Т.е. «please» в данном случае скорее выражает раздражение, чем вежливую просьбу.
Советы немного странные — ну т.е. допустим 2. — вам просто зададут ответный вопрос — а какой класс ты посоветуешь? Далее тупо поправят на это и любая любая ошибка будет на ваш счет
или «7. Обоснуйте ваши предложения» —
вы как бы автоматически ставите себя выше коллеги. это будет работать если это действительно так и он сам с этим согласен, но если руководство подразумевает что вы на одной ступени, то такие «учения» сразу ставят вопрос — «кому следующему поднимать зарплату» не в его пользу
Совет 11 вообще непонятен — ну т.е. вы что-то предлагаете, коллеге это влом делать, возникает вопрос — мы коммитим код в основную ветку или нет? за кем финальное слово? типовой ответ будет — «ну да, я знаю что неоптимально, но сделал так ибо..»
или еще хуже — он с вами начнет обсуждать как лучше, какие варианты, в итоге 2 человека будут заниматься ерундой вместо решения задач проекта. По идее проектный менеджер, если у проекта ограниченный бюджет должен такое пресекать на корню.
или «7. Обоснуйте ваши предложения» —
Когда вы научили коллегу новому подходу
вы как бы автоматически ставите себя выше коллеги. это будет работать если это действительно так и он сам с этим согласен, но если руководство подразумевает что вы на одной ступени, то такие «учения» сразу ставят вопрос — «кому следующему поднимать зарплату» не в его пользу
Совет 11 вообще непонятен — ну т.е. вы что-то предлагаете, коллеге это влом делать, возникает вопрос — мы коммитим код в основную ветку или нет? за кем финальное слово? типовой ответ будет — «ну да, я знаю что неоптимально, но сделал так ибо..»
или еще хуже — он с вами начнет обсуждать как лучше, какие варианты, в итоге 2 человека будут заниматься ерундой вместо решения задач проекта. По идее проектный менеджер, если у проекта ограниченный бюджет должен такое пресекать на корню.
Я сильно привык писать комментарии на английском, и сначала внутренне не согласился, но поразмышлял — и да, это может быть оправдано во многих случаях. Общение на родном языке эффективнее, а на английский переходят для того, чтобы не было проблем при превращении команды в интернациональную.
Но если комментарии в коде, внутренние вики (confluence, jira) и документация имеют долгосрочную ценность, то комментарии при ревью обычно сиюминутны и к ним (по крайней мере, в моей практике) почти не нужно обращаться в будущем. Конечно, бывают важные обсуждения и решения при ревью, но я предпочитаю сводку таких дискуссий переносить в таск-трекер или комментарий в коде.
Но если комментарии в коде, внутренние вики (confluence, jira) и документация имеют долгосрочную ценность, то комментарии при ревью обычно сиюминутны и к ним (по крайней мере, в моей практике) почти не нужно обращаться в будущем. Конечно, бывают важные обсуждения и решения при ревью, но я предпочитаю сводку таких дискуссий переносить в таск-трекер или комментарий в коде.
В статье есть важная фраза:
и это действительно стало практически промышленным стандартом (причины — распределенная команда, разные часовые пояса и прочее). Как и классическая ситуация с огромным количеством изменений в долгоживущей (в которой разработка велась больше 2-х дней) feature ветке, которую ревьювят в пять раундов (либо смотрят поверхностно и не дают дельных замечаний в 2 раунда) — это может длиться неделю с перерывами из-за прокрастинации обоих участников (которая понятна — вся энергия итак потрачена на чтение кода для решения текущей задачи и переключаться на чужие или свои недельные простыни кода уже тяжело) и неважных споров по мелочам.
Для меня было глотком свежего воздуха попробовать синхронное ревью, которое выглядит следующим образом:
ш.1 коллега сбрасывает ссылку на merge request (в нем есть ссылка на задачу)
ш.2 я с задеркжой 0-5-30 (если обед, созвон и проч) минут смотрю код и оставляю замечания — могу часть в личные сообщения, часть в сам merge request. Коллега к новой задаче НЕ приступает (не увеличивает незавершенное производство, нацелен на решение и продвижение текущей задачи)
ш.3 (опционально) — мы созваниваемся с коллегой и проходимся по коду устно. Если есть возможность смержить код (несмотря на замечания) — он мержится (код закрыт feature toggle либо KeystoneInterface — подробнее у Martin Fowler). В любом случае если есть замечания — приступаем к следующем этапу
ш.4 (опционально) коллега правит замечания, я в это время возвращаюсь к своей задаче (я НЕ увеличиваю количество незавершенное работы — так как я уже работал над текущей задаче перед ревью). Но я могу и подождать коллегу и не переключаться
ш.5 возвращаемся к ш.2
Важные условия:
— размер merge request, у нас это ограниение заложено в рамках pipeline — <= 150 строк нового кода
— continious integration. У нас строгий пайплайн и мы часто мержимся
— коллега готов работать в таком режиме. То есть периодически переключаться на то чтобы посмотреть мелкие изменения (оказывается, переключаться чаще на небольшие изменения проще, чем редко на очень больше — также в этой технике не находится места прокрастинации). Обычно МР в этом случае проходит максимум в два раунда и ветка не живет больше 1-2 дней (ci/trunk based development)
— в команде также практикуется парное программирование и парная спецификация задач (ревьювер и автор merge request находятся глубоко в одном контексте)
В нашей команде 2 программиста, но говорят twitter.com/jezhumble/status/1260973883697401856 это масштабируется и на большие команды, тем более если можно дробить команды на более мелкие
Конечно, синхронное ревью (как и парное программирование) звучит очень контринтуитивно в наше время прославления асинхронного общения. Но если есть возможность попробовать и у вас приятные и профессиональные коллеги — почему нет?
Обычно ревью проводят асинхронно.
и это действительно стало практически промышленным стандартом (причины — распределенная команда, разные часовые пояса и прочее). Как и классическая ситуация с огромным количеством изменений в долгоживущей (в которой разработка велась больше 2-х дней) feature ветке, которую ревьювят в пять раундов (либо смотрят поверхностно и не дают дельных замечаний в 2 раунда) — это может длиться неделю с перерывами из-за прокрастинации обоих участников (которая понятна — вся энергия итак потрачена на чтение кода для решения текущей задачи и переключаться на чужие или свои недельные простыни кода уже тяжело) и неважных споров по мелочам.
Для меня было глотком свежего воздуха попробовать синхронное ревью, которое выглядит следующим образом:
ш.1 коллега сбрасывает ссылку на merge request (в нем есть ссылка на задачу)
ш.2 я с задеркжой 0-5-30 (если обед, созвон и проч) минут смотрю код и оставляю замечания — могу часть в личные сообщения, часть в сам merge request. Коллега к новой задаче НЕ приступает (не увеличивает незавершенное производство, нацелен на решение и продвижение текущей задачи)
ш.3 (опционально) — мы созваниваемся с коллегой и проходимся по коду устно. Если есть возможность смержить код (несмотря на замечания) — он мержится (код закрыт feature toggle либо KeystoneInterface — подробнее у Martin Fowler). В любом случае если есть замечания — приступаем к следующем этапу
ш.4 (опционально) коллега правит замечания, я в это время возвращаюсь к своей задаче (я НЕ увеличиваю количество незавершенное работы — так как я уже работал над текущей задаче перед ревью). Но я могу и подождать коллегу и не переключаться
ш.5 возвращаемся к ш.2
Важные условия:
— размер merge request, у нас это ограниение заложено в рамках pipeline — <= 150 строк нового кода
— continious integration. У нас строгий пайплайн и мы часто мержимся
— коллега готов работать в таком режиме. То есть периодически переключаться на то чтобы посмотреть мелкие изменения (оказывается, переключаться чаще на небольшие изменения проще, чем редко на очень больше — также в этой технике не находится места прокрастинации). Обычно МР в этом случае проходит максимум в два раунда и ветка не живет больше 1-2 дней (ci/trunk based development)
— в команде также практикуется парное программирование и парная спецификация задач (ревьювер и автор merge request находятся глубоко в одном контексте)
В нашей команде 2 программиста, но говорят twitter.com/jezhumble/status/1260973883697401856 это масштабируется и на большие команды, тем более если можно дробить команды на более мелкие
Конечно, синхронное ревью (как и парное программирование) звучит очень контринтуитивно в наше время прославления асинхронного общения. Но если есть возможность попробовать и у вас приятные и профессиональные коллеги — почему нет?
Когда все merge request'ы маленькие (а с помощью feature toggles этого можно достичь), то такой подход действительно интересен. Хотя частые переключения ведут к более частой смене контекста, и такое жонглирование может привести к снижению концентрации в течение рабочего дня и повышению усталости в конце его. Впрочем, это индивидуально и необязательно.
Про парное программирование — как-то взаимодействовал с командой из ThoughtWorks, широко использующей практики XP. У них, кстати, Martin Fowler работает. Сам работал в парном режиме с ними; видел, как они друг с другом работает. И сделал такой вывод: если у людей высокая квалификация и они умеют применять парное программирование правильно, то это прям эффективная техника. Иначе это может свестись к тому, что навигатор тупо сидит в телефоне, а драйвер всё молча делает сам, и тогда только вред от такого применения техники.
А долгое время жизни pull request'ов — это боль, да. Особенно, если потом ещё есть дополнительный шаг в виде manual QA check, который становится боттлнеком, если тестировщиков не хватает.
Про парное программирование — как-то взаимодействовал с командой из ThoughtWorks, широко использующей практики XP. У них, кстати, Martin Fowler работает. Сам работал в парном режиме с ними; видел, как они друг с другом работает. И сделал такой вывод: если у людей высокая квалификация и они умеют применять парное программирование правильно, то это прям эффективная техника. Иначе это может свестись к тому, что навигатор тупо сидит в телефоне, а драйвер всё молча делает сам, и тогда только вред от такого применения техники.
А долгое время жизни pull request'ов — это боль, да. Особенно, если потом ещё есть дополнительный шаг в виде manual QA check, который становится боттлнеком, если тестировщиков не хватает.
Хороший ревью сейчас убережёт от технического долга в дальнейшем — это вы классно подметили
Много дельных советов по поводу проведения код ревью, можно найти в этой публикации от гугл. Они вроде бы не открывают америку и многие советы перекликаются с теми, что даны в статье или комментариях, но некоторые моменты могут быть интересными.
Конечно там есть некоторые крайности, на мой взгляд, по поводу сроков, но видимо это от специфики команды зависит.
Конечно там есть некоторые крайности, на мой взгляд, по поводу сроков, но видимо это от специфики команды зависит.
Зарегистрируйтесь на Хабре, чтобы оставить комментарий
Code review Терминатор. Ревью, за которое вам скажут спасибо