[Программирование, Проектирование и рефакторинг, Совершенный код] Поговорим о код-ревью
Автор
Сообщение
news_bot ®
Стаж: 6 лет 9 месяцев
Сообщений: 27286
«Как-то давно мы делали код-ревью, отписывая комменты в почте с указанием номера строк. Это было очень весело. Из плюсов: никто по диффам ничего не смотрел, смотрели в IDE. Но был и минус: после какого-то мержа номера строк менялись».
Александр Макаров, Yii
«В нашей компании есть интересно понятие — стул-реквест. Это когда в рамках одного офиса разработчик подкатывается к тебе на стуле и говорит: „Посмотри, это же быстрее, чем пул-реквест создавать“».
Антон Морев, WormSoft
Недавно на ютубе прошла публичная запись подкаста SDCast о код-ревью. Мы отобрали и расшифровали самое интересное из выпуска.
Полная аудиоверсия на Spotify, Я.Музыке и в виде файла для скачивания
Полная видеоверсия на Youtube
Не относитесь к код-ревью как к проверке чего-то или поиску багов
Сергей Жук, Skyeng: Я считаю, что фундаментальная цель код-ревью — шаринг знаний внутри команды и поиск лучшего решения. Команда просматривает предлагаемые изменения — и знания о домене равномерно распределяются между ее участниками. Автор решения понимает, как код, который он считал идеальным, можно на деле улучшить.
Поэтому код-ревью — не та штука, которую нужно избегать или делать быстрее. Это инвестиции в бизнес и команду: код становится лучше, команда становится лучше. Да, мне сначала не нравилось, когда реквест заворачивали (да и кому такое понравится).
Но затем я стал рассматривать это как обучающий процесс, наравне с чтением книг или участием в конференциях.
Я на себе почувствовал этот плюс, я как разраб вырос с таким подходом.
Но есть нюанс. Наверняка многие из вас сталкивались с реквестами на тысячу строк, где были смешаны и рефакторинг, и фича, и какие-то мелкие изменения. Смешивая в одном реквесте разное, мы усложняем и шаринг знаний, и жизнь ревьюеру: ему тяжелее будет оценить, изменилось ли поведение системы, выполнено ли бизнес-требование, решена ли задача в целом — и удачно ли решение?
Мы у себя в команде этот момент заметили и ввели правило: в одном пул-реквесте не мешать изменения разного характера. Ты отдельно отправляешь рефакторинг, отдельно фичу и отдельно мелкие изменения.
Извините, данный ресурс не поддреживается. :(
Доклад Сергея о практиках его команды. Текстовая версия тут.
Такие реквесты обычно быстро и легко ревьюятся, — и гораздо больше шансов получить качественный фидбек. Да и со стороны мейнтейнера есть плюсы: если рефакторинг нравится, а фича с багом, то можно смержить отдельно рефакторинг.
Александр Макаров: Согласен, что не стоит пытаться тратить минимально возможное время на код-ревью. Открыл, вроде скобки стоят нормально, чего-то этот код делает — так не работает. Если ревьюер не может поручиться до конца за код, значит, код-ревью не проведено.
Поэтому мы сейчас начали составлять себе достаточно большое количество гайдлайнов, и один из них рассказывает про код-ревью.
Часть гайдлайна команды Yii.
А вот к тезису про отдельные маленькие пул-реквесты: у меня были ситуации, когда приходил лид и вводил подобное. Команда воспринимала в штыки. Каким образом у вас это занесли?
Сергей Жук: Была параллельно команда, с которой мы взаимодействовали, юзали их API. Они за месяц сделали кучу фичей, мы чуть-чуть. То есть, изначально мы не на доставку фичей, а именно на качество с таким подходом упирали. И договорились с бизнесом, что релизим медленнее, но стараемся ничего не ломать. Спустя месяц — у соседей то одно сломалось, то другое. А у нас нет. И на этом примере все всё поняли.
Константин Буркалев, SDCast: У меня были процессы внедрения код-ревью в целом — и это было непросто, хотя вроде все понимали, что это хорошо, да. Ты говоришь: «Ребята, давайте мержить через пул-реквест». Тебе говорят: «Да тут маленькая фича».
Тут действительно хорошо работает принцип что, когда что-то сломалось, ты говоришь: „А вот, если бы ты сделал реквест, мы бы посмотрели и до этого просто не дошло“. Идея в том, чтобы люди на собственном опыте поняли ошибки. Пытаться навязать — это не всегда работает.
Как быстро нужно проводить ревью
По ходу трансляции проходили голосования среди зрителей. Вот одно из них.
Константин Буркалев: Джуны особенно часто такие: “Ну что, ты посмотрел мой реквест, нормально там? Я же написал его, кулачки зажал и жду”.
А у меня своя задача, я может только вечерком доберусь или вообще пока не знаю…
Сергей Жук: У нас в команде ревью всегда было приоритетом. Поэтому есть регламент — когда прилетает пул-реквест, ты доделываешь то, чем сейчас занимаешься, чтобы не потерять контекст, и идешь ревьюить. Потому что то, что ты сейчас сделаешь, оно еще в процессе. А та задача уже сделана.
Код уже написан, его осталось только посмотреть, смержить и залить на прод.
То есть, что-то свое докодил, переключился, быстро посмотрел и дальше работать. Наверное, раза по 3 в день я отвлекаюсь от своих задач на ревью. Но, опять же, надо понимать, у меня в команде все делится на маленькие пул-реквесты, по 200-300 строк кода. Соответственно, времени тратится минимум.
«Кто ревьюит — менее важно, чем кого»
Константин Буркалев: Тут напрашивается вопрос — кто должен ревьюить. Только лид? Только сеньор? Или можно и нужно отдавать кому-то ниже по компетенции, просто чтобы человек пытался расти?
А на вопрос, что именно ревьюить, люди ответили так.
Александр Макаров: Если у тебя в команде достаточно много людей, а лид создал из себя бутылочное горлышко, — это тормозит всю команду и в итоге делает команде гораздо хуже. У меня как лида, когда я работал в Skyeng, было на пике 15 пул-реквестов в день, причем не самых маленьких. Я выделял время на ревью утром и вечером.
Ревьюить нужно всех обязательно. За исключением, наверное, историй, где «пожар, все горит» — там уже хуже не будет.
Я лажаю, это нормально, — даже несмотря на то, что я один и тот же проект пилю уже много-много лет. Поэтому сейчас пытаюсь позвать максимальное количество людей посмотреть свой реквест. Это хорошо, так и надо.
Другое дело, всем ли доверять ревью. У меня были ребята, которые могли отлично фигачить, но у них были большие проблемы с фокусом: и, скажем, один тратил на ревью 6 часов. Приходилось учить людей распоряжаться своим временем.
Если речь о коммерческих компаниях, я за то, чтобы обозначить, у кого это обязанность, а кто может ревьюить по пожеланию — и какое время ты в день можешь тратить на ревью, в зависимости от этого статуса.
Антон Морев: Как вижу я — есть разные процессы. Если мы делаем какую-то фичу, которую надо выпустить в сжатый срок, нам не до того, чтобы джуну давать посмотреть код. Но если мы делаем какой-то внутренний продукт или сроки не горят, да — это классная практика, дать джуну отревьюить то, что сделал лид или старший разработчик. Так джуны будут лучше понимать, что происходит в проекте в целом и как устроен механизм принятия решений.
«Это реджект прям сразу»
Сергей Жук: В Skyeng внедрили проверку на коммит-месседж: обязательно должен быть номер задачи в JIRA, иначе пул-реквест не сможешь смержить. Бывает, открываешь код, просто не понимаешь, что там, — открываешь задачу и можно что-то понять.
В остальном, сначала у нас жестко было, потом решили реджектить, только если задача полностью неверно сделана, либо команда архитектурно не согласна. А так: ставим апрув, мержим, пишем замечание — и если автор пул-реквеста захочет, вернется и исправит. Там, поправит мелкие шероховатости или какой-то паттерн использует. А какие у вас практики реджекта?
Александр Макаров: Критерии полностью совпадают с твоим — если поставленная задача выполнена не так, естественно, надо заворачивать. Даже если код выглядит нормально и архитектурно все круто.
Правда, в опенсорсе интереснее: здесь мы часто не реджектим, а стопаем.
Например, говорим: „Ребята, тест давайте“. Есть исключения, конечно, но тесты очень важны. По ним понятно, правильно ли человек понял задачу: опять же, если он тестирует классы и методы, а не юзкейс, это уже подозрительно. Мы сейчас прикрутили infection, это мутационное тестирование. Тулза оставляет те же самые тесты, но модифицирует код и запускает. И если измененный код прошел с теми же тестами, значит, часть кода не нужна — просто можно взять, удалить, ничего не изменится.
Немного бэкстрейджа.
Еще, конечно, вопросы security и performance, — тут будет реджект. Мелочи мы не реджектим: либо просим исправить, либо сами исправляем — пушим прямо в пул-реквесты тех, кто их сделал, и уже показываем на готовом коде, что, как и почему сделали.
Антон Морев: По поводу того, что ты сказал — а решена ли задача. Бывает же, что разработчик, решая одну проблему, решил другую. Вот такие ситуации реджектить не надо. Или как минимум разобраться, как была промодерирована задача.
Константин Буркалев: А мне вот момент связывания коммит-сообщений с таск-трекером кажется важным. Бывают повседневные задачи, в которых это сильно помогает. Знаете, когда: „Слушай, мы что-то похожее делали в рамках задачи про кнопочку“. Ты находишь задачу про кнопочку, понимаешь номер, идешь в лог репозитория, находишь дифф тех коммитов и видишь: действительно, мы прикрутили то-то, это можно перенести сюда.
Но бывает и обратное. Просматриваю я тут исходники одного проекта и встречаю в бэкенде функцию action684.
Пишу товарищу, спрашиваю, это что за классное название в коде такое. Он отвечает — отсылка к таске в трекере. „Зачем придумывать имя функции, ты же пишешь ее в рамках задачи“… Треш, которого в нормальном мире быть не должно, конечно.
===========
Источник:
habr.com
===========
Похожие новости:
- [UML Design, Анализ и проектирование систем, Программирование, Разработка веб-сайтов] UML для самых маленьких: диаграмма классов
- [Программирование] Как и почему компьютеры кидают шулерские кости (перевод)
- [Go, Python, Программирование, Разработка веб-сайтов] Священный холивар «Python vs Go»
- [PHP, ООП, Совершенный код] Вы уверены, что пишете объектно-ориентированный код? (перевод)
- [Карьера в IT-индустрии, Программирование, Учебный процесс в IT] Как из медиа перейти в разработку
- [Open source, Программирование, Python, Социальные сети и сообщества] Spothiefy: как переехать из Яндекс.Музыки быстро, бесплатно
- [Программирование микроконтроллеров, Производство и разработка электроники, Разработка для интернета вещей, Схемотехника, Управление разработкой] Ветроэнергетика — это просто
- [Анализ и проектирование систем, ООП, Программирование, Проектирование и рефакторинг] ООП: Кто взял Измаил? Вопрос принадлежности методов объекту
- [Open source, Rust, Компиляторы, Программирование, Системное программирование] Rust 1.45.0: стабилизация функциональных процедурных макросов, исправление дефектов преобразования (перевод)
- [C#, F#, Программирование] Асинхронность в C# и F#. Подводные камни асинхронности в C #
Теги для поиска: #_programmirovanie (Программирование), #_proektirovanie_i_refaktoring (Проектирование и рефакторинг), #_sovershennyj_kod (Совершенный код), #_praktiki_kodrevju (практики код-ревью), #_cheklist_kodrevju (чек-лист код-ревью), #_antiprimery_kodrevju (антипримеры код-ревью), #_blog_kompanii_skyeng (
Блог компании Skyeng
), #_programmirovanie (
Программирование
), #_proektirovanie_i_refaktoring (
Проектирование и рефакторинг
), #_sovershennyj_kod (
Совершенный код
)
Вы не можете начинать темы
Вы не можете отвечать на сообщения
Вы не можете редактировать свои сообщения
Вы не можете удалять свои сообщения
Вы не можете голосовать в опросах
Вы не можете прикреплять файлы к сообщениям
Вы не можете скачивать файлы
Текущее время: 22-Ноя 19:45
Часовой пояс: UTC + 5
| Автор | Сообщение |
|---|---|
|
news_bot ®
Стаж: 6 лет 9 месяцев |
|
|
«Как-то давно мы делали код-ревью, отписывая комменты в почте с указанием номера строк. Это было очень весело. Из плюсов: никто по диффам ничего не смотрел, смотрели в IDE. Но был и минус: после какого-то мержа номера строк менялись».
Александр Макаров, Yii «В нашей компании есть интересно понятие — стул-реквест. Это когда в рамках одного офиса разработчик подкатывается к тебе на стуле и говорит: „Посмотри, это же быстрее, чем пул-реквест создавать“».
Антон Морев, WormSoft Недавно на ютубе прошла публичная запись подкаста SDCast о код-ревью. Мы отобрали и расшифровали самое интересное из выпуска. Полная аудиоверсия на Spotify, Я.Музыке и в виде файла для скачивания Полная видеоверсия на Youtube Не относитесь к код-ревью как к проверке чего-то или поиску багов Сергей Жук, Skyeng: Я считаю, что фундаментальная цель код-ревью — шаринг знаний внутри команды и поиск лучшего решения. Команда просматривает предлагаемые изменения — и знания о домене равномерно распределяются между ее участниками. Автор решения понимает, как код, который он считал идеальным, можно на деле улучшить. Поэтому код-ревью — не та штука, которую нужно избегать или делать быстрее. Это инвестиции в бизнес и команду: код становится лучше, команда становится лучше. Да, мне сначала не нравилось, когда реквест заворачивали (да и кому такое понравится). Но затем я стал рассматривать это как обучающий процесс, наравне с чтением книг или участием в конференциях. Я на себе почувствовал этот плюс, я как разраб вырос с таким подходом. Но есть нюанс. Наверняка многие из вас сталкивались с реквестами на тысячу строк, где были смешаны и рефакторинг, и фича, и какие-то мелкие изменения. Смешивая в одном реквесте разное, мы усложняем и шаринг знаний, и жизнь ревьюеру: ему тяжелее будет оценить, изменилось ли поведение системы, выполнено ли бизнес-требование, решена ли задача в целом — и удачно ли решение? Мы у себя в команде этот момент заметили и ввели правило: в одном пул-реквесте не мешать изменения разного характера. Ты отдельно отправляешь рефакторинг, отдельно фичу и отдельно мелкие изменения. Извините, данный ресурс не поддреживается. :( Доклад Сергея о практиках его команды. Текстовая версия тут. Такие реквесты обычно быстро и легко ревьюятся, — и гораздо больше шансов получить качественный фидбек. Да и со стороны мейнтейнера есть плюсы: если рефакторинг нравится, а фича с багом, то можно смержить отдельно рефакторинг. Александр Макаров: Согласен, что не стоит пытаться тратить минимально возможное время на код-ревью. Открыл, вроде скобки стоят нормально, чего-то этот код делает — так не работает. Если ревьюер не может поручиться до конца за код, значит, код-ревью не проведено. Поэтому мы сейчас начали составлять себе достаточно большое количество гайдлайнов, и один из них рассказывает про код-ревью.  Часть гайдлайна команды Yii. А вот к тезису про отдельные маленькие пул-реквесты: у меня были ситуации, когда приходил лид и вводил подобное. Команда воспринимала в штыки. Каким образом у вас это занесли? Сергей Жук: Была параллельно команда, с которой мы взаимодействовали, юзали их API. Они за месяц сделали кучу фичей, мы чуть-чуть. То есть, изначально мы не на доставку фичей, а именно на качество с таким подходом упирали. И договорились с бизнесом, что релизим медленнее, но стараемся ничего не ломать. Спустя месяц — у соседей то одно сломалось, то другое. А у нас нет. И на этом примере все всё поняли. Константин Буркалев, SDCast: У меня были процессы внедрения код-ревью в целом — и это было непросто, хотя вроде все понимали, что это хорошо, да. Ты говоришь: «Ребята, давайте мержить через пул-реквест». Тебе говорят: «Да тут маленькая фича». Тут действительно хорошо работает принцип что, когда что-то сломалось, ты говоришь: „А вот, если бы ты сделал реквест, мы бы посмотрели и до этого просто не дошло“. Идея в том, чтобы люди на собственном опыте поняли ошибки. Пытаться навязать — это не всегда работает. Как быстро нужно проводить ревью  По ходу трансляции проходили голосования среди зрителей. Вот одно из них. Константин Буркалев: Джуны особенно часто такие: “Ну что, ты посмотрел мой реквест, нормально там? Я же написал его, кулачки зажал и жду”. А у меня своя задача, я может только вечерком доберусь или вообще пока не знаю… Сергей Жук: У нас в команде ревью всегда было приоритетом. Поэтому есть регламент — когда прилетает пул-реквест, ты доделываешь то, чем сейчас занимаешься, чтобы не потерять контекст, и идешь ревьюить. Потому что то, что ты сейчас сделаешь, оно еще в процессе. А та задача уже сделана. Код уже написан, его осталось только посмотреть, смержить и залить на прод. То есть, что-то свое докодил, переключился, быстро посмотрел и дальше работать. Наверное, раза по 3 в день я отвлекаюсь от своих задач на ревью. Но, опять же, надо понимать, у меня в команде все делится на маленькие пул-реквесты, по 200-300 строк кода. Соответственно, времени тратится минимум. «Кто ревьюит — менее важно, чем кого» Константин Буркалев: Тут напрашивается вопрос — кто должен ревьюить. Только лид? Только сеньор? Или можно и нужно отдавать кому-то ниже по компетенции, просто чтобы человек пытался расти?  А на вопрос, что именно ревьюить, люди ответили так. Александр Макаров: Если у тебя в команде достаточно много людей, а лид создал из себя бутылочное горлышко, — это тормозит всю команду и в итоге делает команде гораздо хуже. У меня как лида, когда я работал в Skyeng, было на пике 15 пул-реквестов в день, причем не самых маленьких. Я выделял время на ревью утром и вечером. Ревьюить нужно всех обязательно. За исключением, наверное, историй, где «пожар, все горит» — там уже хуже не будет. Я лажаю, это нормально, — даже несмотря на то, что я один и тот же проект пилю уже много-много лет. Поэтому сейчас пытаюсь позвать максимальное количество людей посмотреть свой реквест. Это хорошо, так и надо. Другое дело, всем ли доверять ревью. У меня были ребята, которые могли отлично фигачить, но у них были большие проблемы с фокусом: и, скажем, один тратил на ревью 6 часов. Приходилось учить людей распоряжаться своим временем. Если речь о коммерческих компаниях, я за то, чтобы обозначить, у кого это обязанность, а кто может ревьюить по пожеланию — и какое время ты в день можешь тратить на ревью, в зависимости от этого статуса. Антон Морев: Как вижу я — есть разные процессы. Если мы делаем какую-то фичу, которую надо выпустить в сжатый срок, нам не до того, чтобы джуну давать посмотреть код. Но если мы делаем какой-то внутренний продукт или сроки не горят, да — это классная практика, дать джуну отревьюить то, что сделал лид или старший разработчик. Так джуны будут лучше понимать, что происходит в проекте в целом и как устроен механизм принятия решений. «Это реджект прям сразу» Сергей Жук: В Skyeng внедрили проверку на коммит-месседж: обязательно должен быть номер задачи в JIRA, иначе пул-реквест не сможешь смержить. Бывает, открываешь код, просто не понимаешь, что там, — открываешь задачу и можно что-то понять. В остальном, сначала у нас жестко было, потом решили реджектить, только если задача полностью неверно сделана, либо команда архитектурно не согласна. А так: ставим апрув, мержим, пишем замечание — и если автор пул-реквеста захочет, вернется и исправит. Там, поправит мелкие шероховатости или какой-то паттерн использует. А какие у вас практики реджекта? Александр Макаров: Критерии полностью совпадают с твоим — если поставленная задача выполнена не так, естественно, надо заворачивать. Даже если код выглядит нормально и архитектурно все круто. Правда, в опенсорсе интереснее: здесь мы часто не реджектим, а стопаем. Например, говорим: „Ребята, тест давайте“. Есть исключения, конечно, но тесты очень важны. По ним понятно, правильно ли человек понял задачу: опять же, если он тестирует классы и методы, а не юзкейс, это уже подозрительно. Мы сейчас прикрутили infection, это мутационное тестирование. Тулза оставляет те же самые тесты, но модифицирует код и запускает. И если измененный код прошел с теми же тестами, значит, часть кода не нужна — просто можно взять, удалить, ничего не изменится.  Немного бэкстрейджа. Еще, конечно, вопросы security и performance, — тут будет реджект. Мелочи мы не реджектим: либо просим исправить, либо сами исправляем — пушим прямо в пул-реквесты тех, кто их сделал, и уже показываем на готовом коде, что, как и почему сделали. Антон Морев: По поводу того, что ты сказал — а решена ли задача. Бывает же, что разработчик, решая одну проблему, решил другую. Вот такие ситуации реджектить не надо. Или как минимум разобраться, как была промодерирована задача. Константин Буркалев: А мне вот момент связывания коммит-сообщений с таск-трекером кажется важным. Бывают повседневные задачи, в которых это сильно помогает. Знаете, когда: „Слушай, мы что-то похожее делали в рамках задачи про кнопочку“. Ты находишь задачу про кнопочку, понимаешь номер, идешь в лог репозитория, находишь дифф тех коммитов и видишь: действительно, мы прикрутили то-то, это можно перенести сюда. Но бывает и обратное. Просматриваю я тут исходники одного проекта и встречаю в бэкенде функцию action684. Пишу товарищу, спрашиваю, это что за классное название в коде такое. Он отвечает — отсылка к таске в трекере. „Зачем придумывать имя функции, ты же пишешь ее в рамках задачи“… Треш, которого в нормальном мире быть не должно, конечно. =========== Источник: habr.com =========== Похожие новости:
Блог компании Skyeng ), #_programmirovanie ( Программирование ), #_proektirovanie_i_refaktoring ( Проектирование и рефакторинг ), #_sovershennyj_kod ( Совершенный код ) |
|
Вы не можете начинать темы
Вы не можете отвечать на сообщения
Вы не можете редактировать свои сообщения
Вы не можете удалять свои сообщения
Вы не можете голосовать в опросах
Вы не можете прикреплять файлы к сообщениям
Вы не можете скачивать файлы
Вы не можете отвечать на сообщения
Вы не можете редактировать свои сообщения
Вы не можете удалять свои сообщения
Вы не можете голосовать в опросах
Вы не можете прикреплять файлы к сообщениям
Вы не можете скачивать файлы
Текущее время: 22-Ноя 19:45
Часовой пояс: UTC + 5