Если вы работаете в продуктовой компании, то жизненный цикл почти каждого продукта будет соответствовать принципу Парето:
20% времени мы пишем новый код.
80% времени поддерживаем старый. Поддержка в себя включает фиксы багов, обновление кодовой базы (переезд на новые библиотеки например).
Во время поддержки мы хотим чтобы все разработчики как можно быстрее вникали в то, что написано. Для этого есть много способов, все они прекрасны и хорошо работают вместе.
Способы иметь хорошо поддерживаемый код:
Покрытие тестами
Ведение документации
Код ревью
Все эти способы нужно уметь готовить. Есть тесты, которые только мешают, бесполезная документация и бессмысленный код ревью.
Что же такое код ревью?
Основные понятия:
Код ревью (code review) - это часть процесса разработки продукта. В нем есть следующие участники:
Автор - разработчик, который написал код. При этом любого объема, содержания и качества. Это может быть багфикс, фича
Ревьюер - человек, который проверяет написанное, пишет замечания и возвращает код на доработку.
Что нам даёт код ревью:
Проверку кода по многим критериям.
Автор не всегда видит неочевидные места в его коде (по разным причинам).
В результате написания и переписывания может быть потеряна композиция.
Автор, находясь в контексте задачи может недостаточно оставить комментариев.
Эти и многие другие проблемы может решить код ревью.
Шаринг знаний о проекте.Не только автор знает, что там пишется в другом проекте или части проекта, а все.
Шаринг знаний о технологиях.Вы можете заметить, что кто-то напилил своих костылей, а похожая проблема в проекте уже решалась. В таком случае ревьюер может подсказать что уже использовалось и избежать ненужного кода
Сотрудники быстрее онбордятся. Новые сотрудники, проводя код ревью, быстрее узнают и понимают традиции команды, а проходя его, имеют возможность исправить ошибки, узнать больше о продукте и меньше испытывать стресс.
Способов поддерживать код «качественным» есть много, но все их нужно правильно готовить. Нужно уметь писать тесты, нужно уметь писать документацию и нужно правильно организовать код ревью.
Советы по организации процесса
Условные обозначения:
- Как делать не нужно
+ Как делать нужно
- Ревьюить только джунов и новых коллег
+ Проводить ревью для всех и всеми
В коде лидов тоже могут быть ошибки. Усталость, взгляд замылился, бабушка заболела, задачу неправильно понял - причин совершить ошибку может быть много. Коллеги со свежим взглядом помогут вовремя ошибку исправить.
Джуниор разработчики могут подсмотреть какие-то практики, методы, способы из кода сеньора. Вместо того, чтобы собирать их в переговорках и обучать - давайте читать новый код и задавать вопросы. Это уменьшит стоимость обучения и уровень стресса.
Если ревью проходят не все, то в вашей команде это будет восприниматься как источник недоверия и неравенства. Такую атмосферу следует избегать.
- Обсуждать на код ревью стиль
+ Настроить у себя на проекте инструменты, которые автоматически будут исправлять стиль перед коммитом
В JS это eslint и prettier. Не тратьте время своё и коллег на споры о вкусах. Договоритесь заранее и пропишите правила. В случае разногласий голосуйте.
- Ревьюер запускает руками билд или проверяет код на баги.
+ Автор сам несет ответственность за поставленный код.
Проверять код в IDE и запускать код ревьюером - это огромная трата времени. Хорошо налаженный процесс выглядит так:
Автор тщательно проверил свой код на работоспособность и залил в удаленный git репозиторий
На сервере CI/CD проверяет тесты, сборку, работоспособность в целом.
Проверка со стороны ревьюера.
И после исправления замечаний - всё, код отправляется на следующий этап. Если в коде есть ошибки они будут обнаружены отделом тестирования, либо попадут на прод.
Задача ревью не проверить всё на 100% - у вас это все равно не получится. Задача - иметь поддерживаемый, понятный и максимально “чистый” код.
- Ревьюер не читает код
+ Обращать внимание на композицию, магические переменные, оптимизацию (там где это имеет смысл)
Ваша задача, как ревьюера, получить такой код, который в случае болезни разработчика вы завтра будете поддерживать и не потонете. Задача автора - писать код, который как будто будет поддерживать маньяк, который знает где живет автор.
- Агрессия, негатив, «токсичное» поведение в комментах
+ Старайтесь отмечать не только негативные стороны, но и хвалить за позитивные.
Разработчики к своему коду всегда относятся довольно щепетильно. Не зря профессия считается творческой. Поэтому относится к проблемам на код ревью стоит так, как будто вам прислали оценить произведение собственного творчества - стих, картину, книги.
Агрессия, негатив и токсичное поведение в принципе стоит избегать во всех областях жизни. Грубое поведение во время код ревью может привести к:
Конфликтам
Стрессу и страху совершить ошибку
Падению мотивации
Примеры плохих комментов:
«это плохой код», «перепиши»
Примеры хороших комментов:
«Давай попробуем сделать …» «Может попробуем вынести…»
То же самое касается и ответов на комменты автором. Если автор не согласен, то необходимо объяснять с помощью аргументов, а не фраз в духе "я так делать не буду". Если не получается текстом, иногда проще подойти или сделать короткий созвон, чтобы понять друг друга.
- Все комментарии обязательны к исправлению
+ Помечать необязательные комментарии
В замечаниях могут быть необязательные правки - например вкусовщина, или замечания, требующие большую переработку кода. Такие комментарии обязательно помечайте. Если автор не готов их пофиксить сразу, то:
Когда правка относится к вкусовщине - забить
Когда правка большая - завести под нее отдельную таску.
Пытаться объяснить алгоритмы на словах
Написать пример псевдокода
Особенно актуально, если автор не совсем понимает что от него хочет ревьюер. Не нужно полностью писать реализацию. Напишите небольшой пример псевдокода. Gitlab, github и bitbucket позволяют вставлять в комментарии разметку markdown. Ваш код будет хорошо отформатирован и более понятен, чем длинная цепочка комментов.
- Оставлять больше 50 комментариев
+ Оставлять до 50 комментариев
Если комментариев накапливается очень много, то у вас:
Или не настроены линтеры, и вы не определились со стилем, поэтому комменты о вкусовщине
Или переделать в коде нужно многое
Во втором случае оставьте 1 комментарий с рекомендациями как и что нужно переписать. В таком случае лучше закрыть мердж реквест, переписать код и открыть новый.
Если код будет переписан полностью, отследить изменение по комментариям практически невозможно и они не имеют смысла. Авторы тоже должны это понимать и следить, чтобы всем было комфортно.
- Отправлять огромные фрагменты кода на ревью
+ Дробить большие участки кода на несколько реквестов и вливать постепенно
Задачи бывают разными по объему. Может быть задача исправить 1 строчку кода, а может быть задача отрефакторить весь проект. Во втором случае не отправляйте реквест в котором исправлены 500 файлов и 4000 изменений. Никто в здравом уме не сможет это нормально проверить, и желания такое проверять вы тоже не найдете.
Чтобы избежать:
Сделайте ветку feature/epic-name
Изменения делайте в ветке feature/epic-name-implement
Pull реквесты делайте в ветку feature/epic-name. Порционно.
В конце реализации фичи подлейте feature/epic-name в мастер. Да, в этом случае пулл реквест тоже будет огромный, но всё уже будет заранее проверено
Другой вариант как этого избежать: feature flags. Вы вливаете изменения на прод, но не даете им пользоваться. Нормально это реализовано мало у кого. Поэтому с этим подходом нужно быть осторожнее.
- Pull Request висит без ревью трое суток
+ Выработать расписание
Среди аргументов против код ревью вы услышите, что с ним у вас увеличивается время "доставки" фич. Чтобы этого не происходило, выработайте у ревьюеров расписание. Например, новые реквесты проверять до работы и перед уходом домой, а исправленные в перерывах в течении дня (например пока проект собирается или тесты гоняются).
В заключении
В этой статье я хотел рассказать и показать плюсы проведения код ревью. Будучи старшим разработчиком я всегда за то, чтобы мой код проходил code review. Лично для меня это отличный способ “увидеть” свой код чужими глазами. еще до того как ветка отправляется на ревью. Не говоря уже о том, что для команд это недорогой и эффективный способ иметь кодовую базу, с которой можно будет эффективно работать.
Если в вашей команде нет код ревью, то самое время его внедрить .
Ссылки и благодарности
По теме рекомендую почитать:Статья How to Do Code Reviews Like a Human
Спасибо лису и kirjs из @learnInPublic за ревью статьи про ревью.
Комментарии (29)
ivanych
01.09.2021 23:32Проводить ревью для всех и всеми
Всех, но не всеми. Кодревью должны проводить конкретные разработчики, отвечающие за развитие проекта и принимающие решения. Ведущие разработчики.
Смысл в том, что эти разработчики обеспечивают некую линию партии, некий вектор развития, не допуская разброда и шатания.
Код-ревью, проводимый кем попало, приводит к тому, что проект развивается как попало. Каждый начинает лепить кто во что горазд, а ревьюверов это устраивает, потому что они сами имеют разные мнения.
Ну а ведущие разработчики, очевидно, должны ревьювить друг-друга.
symbix
Добавлю. Если у вас большая долгоживущая ветка (вроде упомянутого рефакторинга), в которой вы в ходе работы внесли кучу изменений и фиксов уже написанного кода, перед ревью сделайте интерактивный rebase и скомпонуйте коммиты так, как будто вы с первого раза писали идеальный код. Это тот случай, когда force push абсолютно оправдан (да и все равно долгоживущую ветку вы ребейзили на мастер, не так ли?). Ревьюер отсмотрит код по коммитам (а как ещё такой огромный пул реквест отсматривать?), и ему совершенно неинтересно читать историю ваших косяков.
mapron
… и после ревью, очевидно, так же засквошить «правки по ревью» и «исправление замечаний», никакой ценности для истории они не несут.
symbix
Ну это уже не про ревью, а про историю. А так да, не стал писать ввиду нерелевантности теме.
ApeCoder
Допустим, такая ситуация - у нас есть 10 тематических коммитов, потом по результатам review принято решение переименовать метод который используется во всех них. Вы аккуратно разносите переименования по 10 комитам, чтобы было как будто написано сразу так?
Хотелось бы советов/инструментов по наименее геморройному ребейз интерактиву.
gdt
Лично я бы не стал в каждом из 10 коммитов править, это немножко геморройно (что если их 200, такое тоже бывает). К тому же история она на то и история - тут вот так написал, тут решили переименовать - как раз всё просто и понятно. Тут скорее зависит от соглашений, принятых в команде. Если все перфекционисты, которым нужна идеальная история, и у вас есть время этим заниматься - то почему нет.
mapron
А я бы стал. Если выработать систему, геморроя никакого. 200 не встречал, но даже с 40 коммитами условно проблем нет.
Если вам на ревью по 200 коммитам сразу замечания делают, у вас что-то либо с оформлением истории, либо с системой ревью не так.
Вот мы сейчас пилим долгую жирную фичу. в отдельной ветке. на каждый коммит-два заводится отдельное небольшое ревью. Не надо это все копить.
«ой все может переделаться на 10 раз позже» — ну и что, и переделки тоже отревьюятся отдельно.
gdt
200 коммитов это скорее исключение, чем правило — такой пр невозможно адекватно отсмотреть. Мне вот другое интересно — действительно настолько часто приходится возвращаться к истории, чтобы тратить время на приведение ее в идеальный вид? У меня такое случается раз в несколько месяцев, и в подавляющем большинстве случаев это какой-то один отдельный коммит — т е в принципе неважно, был он приведен в идеальный вид или нет. Вдруг упускаю что-то важное.
mapron
Не знаю к в вашей в работе а у меня это называется мердж реквесты и они есть на КАЖДУЮ фичу, в 75% реквестов есть минимум 1 замечание, значит в 3/4 случаев приходится переоформлять историю.
Т.е. ну не каждый день, но несколько раз в неделю (представьте сами).
Ну это ваш подход в вашей компании, я ж не говорю что у всех одинаково. Лично для меня это раздолбайство, а во многих проектах все эти сотни комитов «fix build»/«fix review» даже не сквошат. (вот пример подобного треша из опенсорс проекта — сравните текст и правки github.com/kleinschrader/kleinsHTTP/commit/ccec65a131a6f6126fc7ce2ef0d4719e093a0b53 (если что mapron даже этого не просил))
gdt
Пулл-реквест или мерж-реквест, по-моему, это просто разные названия для одного и того же. У нас тоже на каждое изменение отдельный ПР, который не будет влит без нескольких аппрувов и успешных сборок/прогонов тестов на CI. Я имею ввиду, вот вы привели историю в идеальный вид, влили, часто ли после этого возвращаетесь к ней?
Чтобы прояснить — я лично каждый раз делаю интерактивный ребейз перед ревью, но не до такой степени, как вы описываете :)
mapron
Простите, я не пытался сделать акцент на термине «мердж-реквест», я тоже оба термина использую взаимозаменяемо. Возможно это была неудачная тонкая шутка на ваше «у меня такое бывает раз в несколько месяцев», не обижайтесь.
Разумеется. У нас продукт долгоиграющий, нет такого что влили фичу и больше её править не приходится. Соответственно когда ты возвращаешься к модулю, git blame это лучший инструмент чтобы быстро освежить в памяти «а ПОЧЕМУ» тут было сделано так, когда комментариев недостаточно. Если вы не пользуетесь git blame/git log в ежедневной работе — то конечно, нафиг не уперлось оформление истории :)
gdt
Да нет, я-то как раз пользуюсь, но видимо не так часто — как уже сказал, раз в несколько месяцев приходится это делать. Может везет с командой, но в большинстве случаев и так все понятно, без дополнительных инструментов.
mapron
А сколько лет кодовой базе над которой вы работаете? 10? 20?
gdt
Продукт разрабатывается с 2005 года. За это время он был несколько раз переписан (последний большой рывок — с C++ на C#), наверное поэтому к древней истории мы практически не возвращаемся, только если в справочных целях.
mapron
Ага! Все так!
Допустим история выглядит так:
— Подфича А
— Подфича Б
— Интеграция А и Б.
Затем по ревью мне написали 2 замечания по подфиче Б и 3 по интеграции. я сделаю такую историю:
— Подфича А
— Подфича Б
— Интеграция А и Б.
+1 Подфча Б — исправил «замечание 1»
+2 Подфча Б — исправил «замечание 2»
+1 Интеграция А и Б. — исправил «замечание 1»
+2 Интеграция А и Б. — исправил «замечание 2»
+3 Интеграция А и Б. — исправил «замечание 3»
(т.о. во время ревью не происходит перезаписывания истории).
После апрува, все это через интерактивный ребейз fixup-ится в соответствующие коммиты.
Никакого геморроя. (главное местами не переставлять).
symbix
В описанном случае не так важно, зависит от подхода команды к чистоте истории.
Я скорее про другого рода случаи.
Допустим, решили отрефакторить длинный метод с парой флагов и почти идентичной копипастой, сделать extract method.
На метод были тесты. Отрефакторили, тесты проходят, но при ручном тестировании видны проблемы: ой, а тест-то был не полный (что неудивительно при таком толстом методе). Увидели, что при рефакторинге внесли баг. Добавили тесткейс, поправили отрефакторенный код.
Сейчас у вас 3 коммита: выделение метода, добавление тесткейса, правка бага.
Прежде, чем отправить на review, я бы сделал rebase: первым коммитом бы шло добавление тесткейса, а вторым — рефакторинг (с засквошенной в него правкой бага).
mapron
Вот хз, я лично за такое по рукам бью и прошу выделять в отдельный коммит. Если нашли баг в ходе ревью, лучше пусть фикс будет точечно и четко отдельным коммитом в истории, никак не связан с перемещениями и переименованиями.
symbix
Не-не. Если баг уже был, то это совершенно другой разговор.
Речь о кейсе
Тут речь о баге, который я внес в новом коде, который в рамках этой ветки и написал, после чего его обнаружил и поправил.
То же относится к любому новому коду, написанному в рамках фичеветки. Зачем эта история косяков разработчика, которые он исправил до мержа? Это в чистом виде мусор.
Да, на таком коротком примере можно просто засквошить все в один коммит прямо при мердже, но давайте представим себе, что в рамках одного рефакторинга таких случаев, ну, 10.
mapron
Понял, согласен, прошу прощения за свой косяк изначально. Вы полностью правы.
olegklimakov Автор
Всё зависит от того как принято проводить ревью в команде.
Компоновать красивые коммиты может быть затратно по времени и через историю коммитов мало кто проверяет пул реквест (по моему опыту).
Чаще всё же через просмотр изменений по всему реквесту.
mapron
Зависит от платформы. например в Bitbucket (ex stash) удобно реквест смотреть как и целиком, так и по коммитам, если их больше пары.
Я постоянно читаю (не только тут в комментах, во всех статьях по ревью), мол оформлять историю затратно, мутно долго и тп, вообще в упор не могу понять этого подхода. Ты колбасил код неделю, отдаешь на ревью и не можешь 10 минут потратить на оформление истории безо всяких эти "+ фикс правки после правок"?
Мне кажется, все разработчики которые спустя рукава и с ленцой относятся к оформлению истории «да кто ее читать будет вообще» в проекте, никогда в своей жизни не пользовались «git blame/git log» для того чтобы понять что было сделано и зачем по какому-то месту в файле. Только не надо вот «для этого должны быть комментарии в коде» — не всегда они есть, даже если все комментируется исправно, и не всегда их достаточно.
olegklimakov Автор
Историю безусловно стоит оформлять, но не в рамках PR. Зачем? Он же все равно будет сквошиться в 1 коммит, в котором будет и описание и номера задач в которых можно посмотреть контекст.
Мне кажется тезиса, что история коммитов не важна в принципе в комментариях не было. Обсуждаем исключительно в рамках пулл реквеста. А там уже каждый сам себе хозяин. Я например могу черновые разработки коммитить, чтобы появилась активность в задаче и ПМ видел что на задачу не забили. Потом вообще переписать этот код и декомпозировать. Поэтому эти коммиты сами по себе никакой ценности не несут. А целиком PR со всеми изменения - несет.
mapron
А зачем открывать PR если задача в статусе черновика и разработка не завершена? ИМХО, стоит оформлять историю в лучшем виде и до открытия реквеста, и после (когда сквошатся замечания).
symbix
Засквошить в один коммит изменения на несколько тысяч строк - это так себе идея.
А для видимости прогресса совершенно ни к чему делать PR, достаточно пушить фичеветку.
olegklimakov Автор
Да, я про это и говорю, что она не всегда пушится валидным кодом, который доживет до PR.
Поинт про интерактивный ребейз я понял, это хорошая мысль. Главное чтобы в компании не было политики - все PR сквошим, тогда в этом отпадает смысл
symbix
Из любого правила есть исключения, здравый смысл всегда должен быть на первом месте.
Если в описанном мной случае кто-то будет настаивать на том, что надо сквошить, потому что надо сквошить, я, скажем так, сильно удивлюсь.
И даже если у кого-то бюрократия головного мозга, в конце концов, ничто не мешает в таком запущенном случае засквошить после ревью.
symbix
Все зависит еще от объема пулл-реквеста.
У меня как-то был пулл-реквест рефакторинга ОЧЕНЬ старого кода на десятки тысяч строк. Отревьюить его целиком просто невозможно: читая весь changeset сверху вниз, просто непонятна логика — особенно учитывая то, что в команде не осталось ни одного из авторов того кода, и уже никто не помнил, как он работает. Поэтому я все разбил на коммиты (их там было порядка 30), и, просматривая коммиты последовательно, ревьюеру становится очевидна логика, которой я руководствовался, какие сущности добавились или модицифировались, какое API добавилось, и как этот API потом используются, какие тесты добавились. Конечно, естественная последовательность коммитов была совершенно другой, и я потратил где-то полчаса на rebase, но ревьюеру не то, что сэкономил часы — я думаю, что без этого вообще никто бы не взялся это полноценно ревьюить, и все бы закончилось чем-то типа "ну блин, офигеть, но если ты уверен, что все хорошо, давай на стейдж, пусть QA тестит".
До этого случая так принято не было, после того, как я это сделал, стало принято — ревьюил тимлид, и ему такой подход понравился :)
olegklimakov Автор
Согласен, подход интересный, попробую в следующий раз на огромных PR :)
ivanych
Бесполезная трата времени. Никто не проверяет по-коммитно, проверяют итоговый дифф.
Поэтому пусть история остаётся историей, Гиту без разницы, сколько коммитов в ветке, а разработчик потом сможет найти все шаги.