Поговорим о код-ревью
«Как-то давно мы делали код-ревью, отписывая комменты в почте с указанием номера строк. Это было очень весело. Из плюсов: никто по диффам ничего не смотрел, смотрели в IDE. Но был и минус: после какого-то мержа номера строк менялись».
Александр Макаров, Yii
«В нашей компании есть интересно понятие — стул-реквест. Это когда в рамках одного офиса разработчик подкатывается к тебе на стуле и говорит: «Посмотри, это же быстрее, чем пул-реквест создавать».
Антон Морев, WormSoft
Недавно на ютубе прошла публичная запись подкаста SDCast о код-ревью. Мы отобрали и расшифровали самое интересное из выпуска.
Полная аудиоверсия на Spotify, Я.Музыке и в виде файла для скачивания
Полная видеоверсия на Youtube
Не относитесь к код-ревью как к проверке чего-то или поиску багов
Сергей Жук, Skyeng: Я считаю, что фундаментальная цель код-ревью — шаринг знаний внутри команды и поиск лучшего решения. Команда просматривает предлагаемые изменения — и знания о домене равномерно распределяются между ее участниками. Автор решения понимает, как код, который он считал идеальным, можно на деле улучшить.
Поэтому код-ревью — не та штука, которую нужно избегать или делать быстрее. Это инвестиции в бизнес и команду: код становится лучше, команда становится лучше. Да, мне сначала не нравилось, когда реквест заворачивали (да и кому такое понравится).
Но затем я стал рассматривать это как обучающий процесс, наравне с чтением книг или участием в конференциях.
Я на себе почувствовал этот плюс, я как разраб вырос с таким подходом.
Но есть нюанс. Наверняка многие из вас сталкивались с реквестами на тысячу строк, где были смешаны и рефакторинг, и фича, и какие-то мелкие изменения. Смешивая в одном реквесте разное, мы усложняем и шаринг знаний, и жизнь ревьюеру: ему тяжелее будет оценить, изменилось ли поведение системы, выполнено ли бизнес-требование, решена ли задача в целом — и удачно ли решение?
Мы у себя в команде этот момент заметили и ввели правило: в одном пул-реквесте не мешать изменения разного характера. Ты отдельно отправляешь рефакторинг, отдельно фичу и отдельно мелкие изменения.
Доклад Сергея о практиках его команды. Текстовая версия тут.
Такие реквесты обычно быстро и легко ревьюятся, — и гораздо больше шансов получить качественный фидбек. Да и со стороны мейнтейнера есть плюсы: если рефакторинг нравится, а фича с багом, то можно смержить отдельно рефакторинг.
Александр Макаров: Согласен, что не стоит пытаться тратить минимально возможное время на код-ревью. Открыл, вроде скобки стоят нормально, чего-то этот код делает — так не работает. Если ревьюер не может поручиться до конца за код, значит, код-ревью не проведено.
Поэтому мы сейчас начали составлять себе достаточно большое количество гайдлайнов, и один из них рассказывает про код-ревью.
Часть гайдлайна команды Yii.
А вот к тезису про отдельные маленькие пул-реквесты: у меня были ситуации, когда приходил лид и вводил подобное. Команда воспринимала в штыки. Каким образом у вас это занесли?
Сергей Жук: Была параллельно команда, с которой мы взаимодействовали, юзали их API. Они за месяц сделали кучу фичей, мы чуть-чуть. То есть, изначально мы не на доставку фичей, а именно на качество с таким подходом упирали. И договорились с бизнесом, что релизим медленнее, но стараемся ничего не ломать. Спустя месяц — у соседей то одно сломалось, то другое. А у нас нет. И на этом примере все всё поняли.
Константин Буркалев, SDCast: У меня были процессы внедрения код-ревью в целом — и это было непросто, хотя вроде все понимали, что это хорошо, да. Ты говоришь: «Ребята, давайте мержить через пул-реквест». Тебе говорят: «Да тут маленькая фича».
Тут действительно хорошо работает принцип что, когда что-то сломалось, ты говоришь: «А вот, если бы ты сделал реквест, мы бы посмотрели и до этого просто не дошло». Идея в том, чтобы люди на собственном опыте поняли ошибки. Пытаться навязать — это не всегда работает.
Как быстро нужно проводить ревью
По ходу трансляции проходили голосования среди зрителей. Вот одно из них.
Константин Буркалев: Джуны особенно часто такие: «Ну что, ты посмотрел мой реквест, нормально там? Я же написал его, кулачки зажал и жду».
А у меня своя задача, я может только вечерком доберусь или вообще пока не знаю…
Сергей Жук: У нас в команде ревью всегда было приоритетом. Поэтому есть регламент — когда прилетает пул-реквест, ты доделываешь то, чем сейчас занимаешься, чтобы не потерять контекст, и идешь ревьюить. Потому что то, что ты сейчас сделаешь, оно еще в процессе. А та задача уже сделана.
Код уже написан, его осталось только посмотреть, смержить и залить на прод.
То есть, что-то свое докодил, переключился, быстро посмотрел и дальше работать. Наверное, раза по 3 в день я отвлекаюсь от своих задач на ревью. Но, опять же, надо понимать, у меня в команде все делится на маленькие пул-реквесты, по 200–300 строк кода. Соответственно, времени тратится минимум.
»Кто ревьюит — менее важно, чем кого»
Константин Буркалев: Тут напрашивается вопрос — кто должен ревьюить. Только лид? Только сеньор? Или можно и нужно отдавать кому-то ниже по компетенции, просто чтобы человек пытался расти?
А на вопрос, что именно ревьюить, люди ответили так.
Александр Макаров: Если у тебя в команде достаточно много людей, а лид создал из себя бутылочное горлышко, — это тормозит всю команду и в итоге делает команде гораздо хуже. У меня как лида, когда я работал в Skyeng, было на пике 15 пул-реквестов в день, причем не самых маленьких. Я выделял время на ревью утром и вечером.
Ревьюить нужно всех обязательно. За исключением, наверное, историй, где »пожар, все горит» — там уже хуже не будет.
Я лажаю, это нормально, — даже несмотря на то, что я один и тот же проект пилю уже много-много лет. Поэтому сейчас пытаюсь позвать максимальное количество людей посмотреть свой реквест. Это хорошо, так и надо.
Другое дело, всем ли доверять ревью. У меня были ребята, которые могли отлично фигачить, но у них были большие проблемы с фокусом: и, скажем, один тратил на ревью 6 часов. Приходилось учить людей распоряжаться своим временем.
Если речь о коммерческих компаниях, я за то, чтобы обозначить, у кого это обязанность, а кто может ревьюить по пожеланию — и какое время ты в день можешь тратить на ревью, в зависимости от этого статуса.
Антон Морев: Как вижу я — есть разные процессы. Если мы делаем какую-то фичу, которую надо выпустить в сжатый срок, нам не до того, чтобы джуну давать посмотреть код. Но если мы делаем какой-то внутренний продукт или сроки не горят, да — это классная практика, дать джуну отревьюить то, что сделал лид или старший разработчик. Так джуны будут лучше понимать, что происходит в проекте в целом и как устроен механизм принятия решений.
»Это реджект прям сразу»
Сергей Жук: В Skyeng внедрили проверку на коммит-месседж: обязательно должен быть номер задачи в JIRA, иначе пул-реквест не сможешь смержить. Бывает, открываешь код, просто не понимаешь, что там, — открываешь задачу и можно что-то понять.
В остальном, сначала у нас жестко было, потом решили реджектить, только если задача полностью неверно сделана, либо команда архитектурно не согласна. А так: ставим апрув, мержим, пишем замечание — и если автор пул-реквеста захочет, вернется и исправит. Там, поправит мелкие шероховатости или какой-то паттерн использует. А какие у вас практики реджекта?
Александр Макаров: Критерии полностью совпадают с твоим — если поставленная задача выполнена не так, естественно, надо заворачивать. Даже если код выглядит нормально и архитектурно все круто.
Правда, в опенсорсе интереснее: здесь мы часто не реджектим, а стопаем.
Например, говорим: «Ребята, тест давайте». Есть исключения, конечно, но тесты очень важны. По ним понятно, правильно ли человек понял задачу: опять же, если он тестирует классы и методы, а не юзкейс, это уже подозрительно. Мы сейчас прикрутили infection, это мутационное тестирование. Тулза оставляет те же самые тесты, но модифицирует код и запускает. И если измененный код прошел с теми же тестами, значит, часть кода не нужна — просто можно взять, удалить, ничего не изменится.
Немного бэкстрейджа.
Еще, конечно, вопросы security и performance, — тут будет реджект. Мелочи мы не реджектим: либо просим исправить, либо сами исправляем — пушим прямо в пул-реквесты тех, кто их сделал, и уже показываем на готовом коде, что, как и почему сделали.
Антон Морев: По поводу того, что ты сказал —, а решена ли задача. Бывает же, что разработчик, решая одну проблему, решил другую. Вот такие ситуации реджектить не надо. Или как минимум разобраться, как была промодерирована задача.
Константин Буркалев: А мне вот момент связывания коммит-сообщений с таск-трекером кажется важным. Бывают повседневные задачи, в которых это сильно помогает. Знаете, когда: «Слушай, мы что-то похожее делали в рамках задачи про кнопочку». Ты находишь задачу про кнопочку, понимаешь номер, идешь в лог репозитория, находишь дифф тех коммитов и видишь: действительно, мы прикрутили то-то, это можно перенести сюда.
Но бывает и обратное. Просматриваю я тут исходники одного проекта и встречаю в бэкенде функцию action684.
Пишу товарищу, спрашиваю, это что за классное название в коде такое. Он отвечает — отсылка к таске в трекере. «Зачем придумывать имя функции, ты же пишешь ее в рамках задачи»… Треш, которого в нормальном мире быть не должно, конечно.
