Что такое код ревью
Что такое код-ревью
Это проверка кода на ошибки, неточности и общий стиль программирования.
Послушать аудиоверсию этой статьи (6 минут):
Слушайте Что такое код-ревью на Яндекс.Музыке
Ситуация: вы разработчик. Вы отвечаете за свой фронт работы, например, за отправку данных на сервер. У команды уже есть готовый проект, вы вместе его поддерживаете и постоянно улучшаете.
Когда вы пишете новую функцию, она не попадает сразу в проект. Вместо этого ваш код отправляется на код-ревью (code review).
Что делают на код-ревью
Во время код-ревью кто-то из старших товарищей изучает ваш код на предмет:
Если проблемы есть, проверяющий отправляет код на доработку. Если всё хорошо, код переходит на следующую стадию — как правило, в тестирование.
Кто проводит
Обычно принято так: код-ревью делают программисты более старшего уровня. Джуниоров проверяют мидлы, мидлов — сеньоры, сеньоров — другие сеньоры или тимлиды. Если компания большая, то могут выделить отдельно несколько человек, которые смотрят код у всех и следят за общим стилем.
Если команда небольшая, то код-ревью делает ведущий программист — он сам следит за проектом и за качеством кода, который пишут остальные.
Как это выглядит
Зачем это нужно
Если вы пишете один или с другом, то, скорее всего, вам это не нужно. Вы и так можете обсудить код между собой и понять, как его сделать лучше.
Когда в команде программистов много, то компания сталкивается с тем, что все пишут по-разному. И даже если весь этот код работает, его потом нужно поддерживать, а ковыряться в чужом коде, если он плохо написан — это долго и дорого. Поэтому на этапе код-ревью разработчики делают так, чтобы им же позднее было проще поддерживать код и ничего не ломалось.
Code Review – зачем и как использовать в команде?
Что такое Code Review
Зачем нужен Code Review
Code Review может являться частью процесса выполнения задачи (частью workflow). Может показаться, что ревьювить должен только тимлид или старший разработчик, но хорошей практикой является если в процессе ревью задач участвуют все разработчики. Таким образом можно не только распределить нагрузку от ревью, но и составить у команды более широкое представление о выполняемых задачах. Также это помогает делиться best practices внутри команды.
Положительные эффекты в команде от Code Review:
понижает bus factor: больше людей в команде в курсе выполняемой задачи, в случае необходимости внесения изменений в задачу как минимум два человека смогут это сделать. Задача больше не завязана на одного разработчика.
помогает найти и выявить баги и недоработки на этапе разработки задачи: так как задача сразу проверяется как минимум двумя разработчиками, это повышает вероятность нахождения упущенных кейсов, которые без код ревью могли бы попасть на бой.
обучаемость сотрудников: разные реализации и подходы к решению задач могут заимствоваться участниками команды друг у друга во время код ревью
развитие и поддержание здоровой культуры в команде: участники команды учатся друг у друга и учатся давать качественную обратную связь, повышается взаимодействие внутри команды.
при разработке задачи на реализацию тратится чуть больше времени
в задаче задействованы как минимум два разработчика (тот, кто делал задачу и тот, кто ее ревьювил)
Рекомендации по организации Code Review
Code Review может быть организован по-разному в разных командах. Главное, чтобы команда заранее обговорила и утвердила свои внутренние правила, которых она хочет придерживаться и с которыми все согласны, чтобы каждый раз не возвращаться к этому вопросу.
Избегать рутинных проверок Code Style людьми: автоматизировать такие проверки. Можно использовать для этого любые подходящие вам инструменты для автоматической проверки code style используемого вами языка программирования. Например, для PHP это может быть PHP Coding Standards Fixer https://cs.symfony.com/ Не нужно тратить время разработчиков на то, что можно автоматизировать. Также обратная связь по поводу code style от людей воспринимается как “придирки” и может создать не очень позитивную атмосферу в команде.
Code Review должен проводиться для каждого участника команды, вне зависимости от уровня. Не должно быть такого, что ревьювят только задачи, которые сделали Junior разработчики, тем временем Senior разработчики не отдают свои задачи на ревью. Необходимо, чтобы ревью проводилось для задач всех разработчиков.
Code Review является частью процесса и необходим каждой задаче. Это правило избавляет от лишних споров и холиваров насчет небольших задач. Ревью проходят все задачи без исключений.
Code Review проводится перед релизом задачи и перед передачей ее в тестирование. Это помогает избегать повторного тестирования, а также соблазна оставить все как есть, ведь “и так работает”. К задачам, которые уже на бою, никто не захочет повторно возвращаться.
Избегать слишком больших объемов кода в одном Code Review. Если задача большая, то необходимо отправлять ее на ревью частями. Есть рекомендуемое число 200-400 строк в одном ревью. При увеличении количества строк, эффективность и продуктивность ревью резко падает.
Если нет возможности внести какие-то правки после ревью, то необходимо завести задачу в трекере задач, а в коде оставить ToDo с ее номером
Чего следует избегать:
Если Code Review непостоянная часть процесса разработки, то это приведет к нестабильному ревью, его будут откладывать и команда не получит всех плюсов этого процесса.
Старшие разработчики рвьювят новых и младших разработчиков. Это создает плохую культуру в команде, а также не позволяет младшим разработчикам увидеть какие-то решения старших разработчиков, на которых они могли бы поучиться и узнать что-то новое.
Не автоматизировать процесс ревью и не использовать сторонние инструменты для ревью. Никто не любит заниматься рутинными процессами. Нужно автоматизировать все, что можно автоматизировать.
На что обращать внимание во время Code Review
Чеклист для разработчика перед отправкой на ревью:
Проверить, что реализация соответствует всем указанным в исходной задаче условиям
Проверить соответствие Code Style и другим принятым в команде гайдлайнам, например, наличию unit-тестов и документации
Протестировать задачу локально и убедиться, что она работает, как нужно
Подготовить описание для ревьювера, если какой-то информации в задаче не хватает
Проверить, нужны ли какие-то комментарии в самом коде и добавить при необходимости
Чеклист для ревьювера:
Ознакомиться и понять цель и суть задачи
Проверить общую архитектуру и подход к решению
Проверить мелкие детали (имена функций и переменных и т.д.)
Проверить наличие тестов и документации по необходимости
Список ToDo: изменения, которые необходимо внести в код после ревью
Вопросы: обозначить свои вопросы по частям кода, которые непонятны после ревью
Предложения по улучшению: внести свои предложения и пожелания по коду задачи и/или связанных задач. Например, договориться о создании задачи по обновлению старого метода в других участках кода на новый и завести на это ToDo и задачу в трекере задач и поставить ее в тех. долг команды.
Также отдельно хочется отметить, что если вы ревьювите чью-то задачу и видите какие-то хорошие подходы и решения, то скажите об это автору. Положительная обратная связь тоже очень важна.
Инструменты для Code Review
Поищите инструменты для вашего языка программирования. Используйте тот, который больше всего подойдет вашей команде.
Ищем смысл в код-ревью
«Как-то давно мы делали код-ревью, отписывая комменты в почте с указанием номера строк. Это было очень весело. Из плюсов: никто по диффам ничего не смотрел, смотрели в IDE. Но был и минус: после какого-то мержа номера строк менялись».
Александр Макаров, Yii
«В нашей компании есть интересно понятие — стул-реквест. Это когда в рамках одного офиса разработчик подкатывается к тебе на стуле и говорит: „Посмотри, это же быстрее, чем пул-реквест создавать“».
Антон Морев, WormSoft
Недавно на ютубе прошла публичная запись подкаста SDCast о код-ревью. Мы отобрали и расшифровали самое интересное из выпуска.
Полная аудиоверсия на Spotify, Я.Музыке и в виде файла для скачивания
Полная видеоверсия на Youtube
Не относитесь к код-ревью как к проверке чего-то или поиску багов
Сергей Жук, Skyeng: Я считаю, что фундаментальная цель код-ревью — шаринг знаний внутри команды и поиск лучшего решения. Команда просматривает предлагаемые изменения — и знания о домене равномерно распределяются между ее участниками. Автор решения понимает, как код, который он считал идеальным, можно на деле улучшить.
Поэтому код-ревью — не та штука, которую нужно избегать или делать быстрее. Это инвестиции в бизнес и команду: код становится лучше, команда становится лучше. Да, мне сначала не нравилось, когда реквест заворачивали (да и кому такое понравится).
Но затем я стал рассматривать это как обучающий процесс, наравне с чтением книг или участием в конференциях.
Я на себе почувствовал этот плюс, я как разраб вырос с таким подходом.
Но есть нюанс. Наверняка многие из вас сталкивались с реквестами на тысячу строк, где были смешаны и рефакторинг, и фича, и какие-то мелкие изменения. Смешивая в одном реквесте разное, мы усложняем и шаринг знаний, и жизнь ревьюеру: ему тяжелее будет оценить, изменилось ли поведение системы, выполнено ли бизнес-требование, решена ли задача в целом — и удачно ли решение?
Мы у себя в команде этот момент заметили и ввели правило: в одном пул-реквесте не мешать изменения разного характера. Ты отдельно отправляешь рефакторинг, отдельно фичу и отдельно мелкие изменения.
Доклад Сергея о практиках его команды. Текстовая версия тут.
Такие реквесты обычно быстро и легко ревьюятся, — и гораздо больше шансов получить качественный фидбек. Да и со стороны мейнтейнера есть плюсы: если рефакторинг нравится, а фича с багом, то можно смержить отдельно рефакторинг.
Александр Макаров: Согласен, что не стоит пытаться тратить минимально возможное время на код-ревью. Открыл, вроде скобки стоят нормально, чего-то этот код делает — так не работает. Если ревьюер не может поручиться до конца за код, значит, код-ревью не проведено.
Поэтому мы сейчас начали составлять себе достаточно большое количество гайдлайнов, и один из них рассказывает про код-ревью.
А вот к тезису про отдельные маленькие пул-реквесты: у меня были ситуации, когда приходил лид и вводил подобное. Команда воспринимала в штыки. Каким образом у вас это занесли?
Сергей Жук: Была параллельно команда, с которой мы взаимодействовали, юзали их API. Они за месяц сделали кучу фичей, мы чуть-чуть. То есть, изначально мы не на доставку фичей, а именно на качество с таким подходом упирали. И договорились с бизнесом, что релизим медленнее, но стараемся ничего не ломать. Спустя месяц — у соседей то одно сломалось, то другое. А у нас нет. И на этом примере все всё поняли.
Константин Буркалев, SDCast: У меня были процессы внедрения код-ревью в целом — и это было непросто, хотя вроде все понимали, что это хорошо, да. Ты говоришь: «Ребята, давайте мержить через пул-реквест». Тебе говорят: «Да тут маленькая фича».
Тут действительно хорошо работает принцип что, когда что-то сломалось, ты говоришь: „А вот, если бы ты сделал реквест, мы бы посмотрели и до этого просто не дошло“. Идея в том, чтобы люди на собственном опыте поняли ошибки. Пытаться навязать — это не всегда работает.
Как быстро нужно проводить ревью
По ходу трансляции проходили голосования среди зрителей. Вот одно из них.
Константин Буркалев: Джуны особенно часто такие: “Ну что, ты посмотрел мой реквест, нормально там? Я же написал его, кулачки зажал и жду”.
А у меня своя задача, я может только вечерком доберусь или вообще пока не знаю…
Сергей Жук: У нас в команде ревью всегда было приоритетом. Поэтому есть регламент — когда прилетает пул-реквест, ты доделываешь то, чем сейчас занимаешься, чтобы не потерять контекст, и идешь ревьюить. Потому что то, что ты сейчас сделаешь, оно еще в процессе. А та задача уже сделана.
Код уже написан, его осталось только посмотреть, смержить и залить на прод.
То есть, что-то свое докодил, переключился, быстро посмотрел и дальше работать. Наверное, раза по 3 в день я отвлекаюсь от своих задач на ревью. Но, опять же, надо понимать, у меня в команде все делится на маленькие пул-реквесты, по 200-300 строк кода. Соответственно, времени тратится минимум.
«Кто ревьюит — менее важно, чем кого»
Константин Буркалев: Тут напрашивается вопрос — кто должен ревьюить. Только лид? Только сеньор? Или можно и нужно отдавать кому-то ниже по компетенции, просто чтобы человек пытался расти?
А на вопрос, что именно ревьюить, люди ответили так.
Александр Макаров: Если у тебя в команде достаточно много людей, а лид создал из себя бутылочное горлышко, — это тормозит всю команду и в итоге делает команде гораздо хуже. У меня как лида, когда я работал в Skyeng, было на пике 15 пул-реквестов в день, причем не самых маленьких. Я выделял время на ревью утром и вечером.
Ревьюить нужно всех обязательно. За исключением, наверное, историй, где «пожар, все горит» — там уже хуже не будет.
Я лажаю, это нормально, — даже несмотря на то, что я один и тот же проект пилю уже много-много лет. Поэтому сейчас пытаюсь позвать максимальное количество людей посмотреть свой реквест. Это хорошо, так и надо.
Другое дело, всем ли доверять ревью. У меня были ребята, которые могли отлично фигачить, но у них были большие проблемы с фокусом: и, скажем, один тратил на ревью 6 часов. Приходилось учить людей распоряжаться своим временем.
Если речь о коммерческих компаниях, я за то, чтобы обозначить, у кого это обязанность, а кто может ревьюить по пожеланию — и какое время ты в день можешь тратить на ревью, в зависимости от этого статуса.
Антон Морев: Как вижу я — есть разные процессы. Если мы делаем какую-то фичу, которую надо выпустить в сжатый срок, нам не до того, чтобы джуну давать посмотреть код. Но если мы делаем какой-то внутренний продукт или сроки не горят, да — это классная практика, дать джуну отревьюить то, что сделал лид или старший разработчик. Так джуны будут лучше понимать, что происходит в проекте в целом и как устроен механизм принятия решений.
«Это реджект прям сразу»
Сергей Жук: В Skyeng внедрили проверку на коммит-месседж: обязательно должен быть номер задачи в JIRA, иначе пул-реквест не сможешь смержить. Бывает, открываешь код, просто не понимаешь, что там, — открываешь задачу и можно что-то понять.
В остальном, сначала у нас жестко было, потом решили реджектить, только если задача полностью неверно сделана, либо команда архитектурно не согласна. А так: ставим апрув, мержим, пишем замечание — и если автор пул-реквеста захочет, вернется и исправит. Там, поправит мелкие шероховатости или какой-то паттерн использует. А какие у вас практики реджекта?
Александр Макаров: Критерии полностью совпадают с твоим — если поставленная задача выполнена не так, естественно, надо заворачивать. Даже если код выглядит нормально и архитектурно все круто.
Правда, в опенсорсе интереснее: здесь мы часто не реджектим, а стопаем.
Например, говорим: „Ребята, тест давайте“. Есть исключения, конечно, но тесты очень важны. По ним понятно, правильно ли человек понял задачу: опять же, если он тестирует классы и методы, а не юзкейс, это уже подозрительно. Мы сейчас прикрутили infection, это мутационное тестирование. Тулза оставляет те же самые тесты, но модифицирует код и запускает. И если измененный код прошел с теми же тестами, значит, часть кода не нужна — просто можно взять, удалить, ничего не изменится.
Немного бэкстрейджа.
Еще, конечно, вопросы security и performance, — тут будет реджект. Мелочи мы не реджектим: либо просим исправить, либо сами исправляем — пушим прямо в пул-реквесты тех, кто их сделал, и уже показываем на готовом коде, что, как и почему сделали.
Антон Морев: По поводу того, что ты сказал — а решена ли задача. Бывает же, что разработчик, решая одну проблему, решил другую. Вот такие ситуации реджектить не надо. Или как минимум разобраться, как была промодерирована задача.
Константин Буркалев: А мне вот момент связывания коммит-сообщений с таск-трекером кажется важным. Бывают повседневные задачи, в которых это сильно помогает. Знаете, когда: „Слушай, мы что-то похожее делали в рамках задачи про кнопочку“. Ты находишь задачу про кнопочку, понимаешь номер, идешь в лог репозитория, находишь дифф тех коммитов и видишь: действительно, мы прикрутили то-то, это можно перенести сюда.
Code review — улучшаем процесс
Представим команду, где не проводится Code review. Разработчики пишут код, и без проверок вносят все изменения в основную ветку. Спустя время расширяется функционал или находятся баги, они возвращаются к исходному коду, а там все переменные названы одной буквой, нет следования архитектуре, да и качество не самое лучшее. Этот некачественный код будет копиться и однажды наступит момент, когда, при любом мало-мальском нововведении, проект начнёт разваливаться: в лучшем случае – увеличится время разработки, в худшем – поддержка проекта станет невозможной. И это при том, что когда-то давно задача была выполнена и все хорошо работало.
Как этого можно избежать? Ответ на вопрос в названии – Code review.
Code review — это проверка кода на ошибки, повышающая стабильность работы и качество кода.
Pull request / Merge request — это запрос к команде проекта (человеку или группе людей) на одобрение и применение изменений в выбранную ветку. Как только Pull request будет создан, перед одобрением происходит обсуждение написанного кода. Во время обсуждения могут быть предложены правки. После одобрения текущие изменения попадают в выбранную ветку.
Ниже перечислены рекомендации, которые помогут ускорить Code review и повысить его качество.
Поделим вопрос на три части и рассмотрим каждую по отдельности:
Часть 1. Проверяем код
Запускаем код автора у себя
Запустите код у себя и посмотрите, как изменения работают в связке с остальным кодом. Это помогает найти проблемные места, которые не видны в web-интерфейсе. Старайтесь видеть код комплексно, избегайте фокусироваться только на локальных изменениях. Так Вы быстрее разберетесь с кодом и быстрее найдете архитектурные неточности, если такие есть.
Помним про пользовательский опыт
Смотрим на общую логику
Разработчики могут успешно решить свою задачу, но поломать работу других кусков кода. Чтобы такого не происходило, смотрите не только на то, как решается конкретная задача, но и на то, как изменения отразятся на работе других сервисов, модулей и всего проекта в целом.
Смотрим на архитектуру кода
Архитектура кода определяет, как много времени в будущем мы потратим на расширение, добавление функционала или правку бага. Также архитектура кода может повлиять на потенциальное появление багов в случае изменений в проекте. В идеале расширение и добавление нового функционала не должно приводить к рефакторингу.
Пишем проще
Обращайте внимание на переусложнение кода: чем проще код, тем легче он читается и проще его поддерживать. Избавляйтесь от сложных кусков кода.
Многопоточность
Если в проекте подразумевается работа в нескольких потоках, то смотрите, что будет если во время выполнения кода в каком-то из потоков случится задержка, и как отрабатываются подобные кейсы.
Излишняя оптимизация
Как писал классик Дональд Кнут, преждевременная оптимизация – корень всех зол. Оптимизировать лучше только то, что нужно здесь и сейчас.
Отрабатываем ошибки
Обратите внимание, как поведет себя проект, если не удалось выполнить строчку кода, блок кода или запрос на сервер. Часто разработчики прерывают выполнение функции без вывода ошибок, но подобные кейсы необходимо прорабатывать.
Соответствие договоренностям
Код должен соответствовать договоренностям, код стайлу. Единообразие кода – не прихоть, а необходимость: такой код легче поддерживать, и в таком коде легче разбираться.
Наименования и внешний вид
Помните о других программистах, которым придется разбираться в вашем коде. Читабельный код упрощает его дальнейшую поддержку. Имена должны быть понятные и точно описывать класс, объект, функцию и т.д.
Комментарии к коду
Комментарии должны отвечать на вопрос: «почему так сделано?», а не «как это сделано?». Запомните это.
Часть 2. Обсуждаем
Главное правило обсуждения: любой комментарий должен быть нацелен на код, а не на человека, который его написал. Работает и в обратную сторону – не стоит воспринимать комментарии как критику Вас лично. Задача code review сделать Ваш код лучше, ведь то, что не заметили Вы, могут заметить другие. Коллеги могут предложить альтернативные решения, и в этом заключен потенциал для профессионального роста. Важно помнить, что обсуждение кода – это не состязание в остроумии и не показательное шоу «кто больше знает», поэтому сарказм, скрытая агрессия и хамство в нем неуместны.
Как правило, pull request проводят на специальных web-хостингах (github.com, bitbucket.org, gitlab.com и др.), где есть возможность просмотреть изменения и оставить комментарий к определенному фрагменту кода.
Соблюдаем договоренности
Повторим, код должен соответствовать договоренностям. Однако если таких договоренностей не существует, не стоит просить коллегу добавить пробел или отступ в коде.
В спорных моментах можно договориться всей командой прямо в процессе code review, но просить соблюдать эти правила лучше на следующих code review, так всем будет легче их принять. Кстати, задокументированный гайдлайн по стилю написания убирает практически все споры.
Предлагаем альтернативу
Избегайте высказываний типа «ты сделал не так. », «зачем, почему ты так пишешь?» и др. Они воспринимаются как критика и ведут к оправданиям. Лучше писать комментарий про содержание кода, без обращения к личности автора. Также старайтесь избегать приказов и принуждений: люди не любят, когда им кто-то приказывает, и воспринимают такие комментарии негативно.
Можно действовать по следующей схеме:
Остаемся в рамках задачи
Часто можно увидеть комментарии к коду, который был раньше и никак не трогался. Не надо комментировать то, что не относится к задаче. Любые сторонние правки могут занять много времени, да и восприниматься могут негативно, поэтому лучше смотреть на то, как человек выполнил текущую задачу, а не просить его рефакторить проект.
Хвалим
Если видите интересное или крутое решение, не стесняйтесь хвалить. К тому же, это отличная мотивация для ваших коллег продолжать писать хороший код в дальнейшем.
Все комментарии равны
Часто бывает, что один программист технически знает больше другого, что подтверждается градацией junior, middle, senior, team lead. Не стоит выделять комментарии одной группы как более важные. Это обесценивает мнение части разработчиков, что может привести к равнодушию и формальному участию в code review. Когда мнение всех одинаково важное, code review проходит продуктивнее.
Четко выражаем свои мысли
Для продуктивного общения пишите максимально развернуто и объясняйте каждую деталь. У всех разный уровень знания, а читать мысли еще пока никто не научился.
Задаем вопросы
Не стесняйтесь спрашивать своих коллег, чем их предложенный вариант лучше текущего вашего. Это отличная возможность узнать что-то новое и вырасти профессионально.
Решаем конфликты
Случается, что человек не принимает все доводы и своих предложить не может, отказывается что-то делать. Несколько практических советов на этот случай:
Часть 3. Улучшаем процесс
Описываем, что сделали
Описываем в заголовке pull request (или выносим в отдельный комментарий) суть задачи и какие шаги были проделаны для ее выполнения.
Делим pull request на части
Большой кусок будут долго смотреть, долго обсуждать и долго исправлять. Поделите код на небольшие логические части – так процесс пойдет гораздо быстрее.
Отвечаем на все комментарии
Желательно отвечать на каждый комментарий, чтобы в команде не возникало недоговоренностей. Другие разработчики должны понимать, что Вы прочитали их комментарий, проделали необходимую работу и внесли исправления. Постоянно открывать pull request и смотреть, что было поправлено, а что нет, очень неудобно и отнимает много времени.
Искать все?
Существуют разные подходы – искать максимум из возможного или комментировать сначала важные архитектурные моменты, а после исправления обращать внимание на мелочи.
Оба способа имеют право на жизнь. Я считаю, что второй вариант более трудозатратный: представьте, что после исправления надо полностью просматривать код еще раз, комментировать и снова ждать исправлений. Куда быстрее тщательно пройтись по коду один раз, оставить комментарии и потом проверить исправления.
Если есть архитектурные неточности и понятно, что мелкие ошибки сами исчезнут после исправления архитектуры, не стоит тратить время на комментирование мелочей в этом участке кода.
Ждать всех?
Можно не ждать пока pull request одобрят все и договориться, что одобрения 80% ревьюверов достаточно для закрытия задачи. Это ускорит попадание кода в основную ветку, что несомненно более выгодно для бизнес-процессов, но может привести к разногласиям в команде и равнодушию к code review.
Второй вариант – обязательно ждать одобрения всех причастных разработчиков. Качество кода вырастет, но сам процесс замедлится значительно. Выбор в пользу скорости или качества каждая команда должна принимать самостоятельно.
Мелочи
Если к коду нет серьезных замечаний, то не нужно ждать, когда будут убраны все маленькие неточности. Их можно указать в комментарии и сразу одобрить pull request – автор кода будет чувствовать себя спокойнее, повысится его лояльность к команде, почувствует, что ему доверяют. И, конечно, скорость прохождения pull request возрастет.