Comments 31
Есть ощущение, что спутан процесс code review и непосредственно анализ изменений, как процесс аналитический. Инструменты приведенные в конце списка - IMHO из разных категорий.
Было бы хорошо описать, зачем нужны именно эти инструменты, так как изначально code review system - это немного другие инструменты, а именно github, bitbucket, gerrit. И если взять и сравнить атлассиновские Bitbucket, Crucible (сюда можно еще и Fisheye добавить), то все они могут использоваться для просмотра и сравнения кода (code review). Но если говорить о code review как часть процесса в SDLC, то в современной разработке основное все-же BitBucket, и если говорить про пулл реквесты и код ревью пулл реквестов, статья должна быть немного другая.
Другими словами - в статье описан code review, который должен являться частью SDLC (и описан поверхностно), а инструменты приведены для другой задачи - более глубокой аналитике во просмотра изменений в коде, причем не обязательно связанных с пулл/мерж реквестами. Например при помощи Crucible можно сравнить не изменения в конкретном коммите, а два бренча, которые давно разошлись друг с другом и имеют сотни изменений.
Не соглашусь. Здесь именно про Code Review и инструменты в конце статьи могут использоваться для Code Review в командах (это не значит, что они только для этого). Более того, был опыт использования большинства из них при ревью в разных компаниях. Кстати, про Fisheye хорошее дополнение.
Но вся суть – не в инструментах, а в подходе. Можно ревьювить код задачи без специальных инструментов, а просто переключаясь в ветку задачи и смотря комиты по ней в в вашей текущей IDE. Инструменты я привела просто в качестве примеров, если вдруг кому-то нужно будет.
Во вы перечислили инструменты аналитики, а ниже в комментариях пишете, что на самом деле в текущем проекте используете обычные пулл реквесты и
Сейчас используем Pull Requests в GitHub для самого процесса ревью
Но это же немного другое.
Пулл реквест это функционал, который встраивается в CI/CD систему, на пулл реквест можно навешать различные тесты, анализаторы и так далее. И да, конечно там тоже есть просмотр диффа, возможность комментирования аппрувалы.
Но вот список софта, который приводился в статье - он в первую очередь об удобом сравнении множества разных коммитов для просмотра их человеком.
Например банальный вещь, которая реализуется через pull request - автоматическая проверка названия фича бренча, чтобы он соответствовал нейм конвеншену, и проверка коммит сообщения, чтобы оно, например, начиналось с номера тикета в JIRA, запуск компиляции и даже интеграционных тестов, и автоматический аппрувал от билд системы, который не позволит замержить пулл реквест, пока билд не успешен.
А такая вещь, как Crucible или FishEye - это совсем не для этого.
Я перечислила примеры инструментов, которые могут использоваться для Code Review. И буквально первая ссылка в этом списке – ссылка на GitHub, где написано про Pull Requests: "Pull requests are fundamental to how teams review and improve code on GitHub."
Для всех остальных упомянутых в статье инструментов также на главных страницах по ссылкам явно написано, что они используются в том числе для Code Review. Про Crucible опять же буквально написано в официальной документации "Crucible allows you to request, perform and manage code reviews."
То, что можно настроить CI/CD, это прекрасно, но это никак не относится ни к теме статьи, ни к моим комментариям, ни к тому как эта статья задумывалась. И то, что указанные инструменты можно еще как-то использовать, помимо непосредственно Code Review – тоже. Я нигде не пишу, что это их единственное назначение и/или что это все инструменты для ревью, которые есть. Не было цели описать инструменты и все кейсы их использования.
Если для вас в статье нет ничего нового, а указанные инструменты вы используете иначе – это ок. Я не вижу тут причины для споров. Просто, возможно, вы не целевая аудитория этой статьи.
Полезные заметки по процессу и путях улучшения Code Review.
Не хватает реального опыта вместо теории.
А то у нас в команде процесс ревью - это скорее то, что никто не хочет делать (скучно) и если бы автор поделился реальным опытом, о том как сделает проще и быстрее, то эта статья была бы гораздо полезнее.
А что именно про опыт? Часть вот этих чеклистов мы, например, использовали в команде, прям писали их даже во внутренней документации для новых сотрудников, которые раньше не делали ревью. Сейчас используем Pull Requests в GitHub для самого процесса ревью, большие задачи стараемся комитить мелкими частями. Эта статья именно больше про теорию и задумывалась. Но могу и про опыт написать отдельно, мне просто кажется, что там будет мало.
Опыт автоматизации, примеры проблема - решение.
Был бы интересен такой формат :
Проблема :
В команде процесс код ревью растягивал релиз на +1 или +2 недели, часто давали заниженный эстимейт. Как результат : негативный фидбек от клиента и настроение команды.
Решение :
Взяли за правило добавлять 20% от начального эстимейта в качестве буфера для код ревью. Да, шанс промахнутся из-за код ревью остался, но разница будет существенно ниже. Как результат : время на ревью больше, результат ревью качественней, команда в положительном настроении, клиент не переживает.
"на реализацию тратится чуть больше времени" - во сколько вы оцениваете это "чуть"?
Если задачи хорошо декомпозированы, то временные затраты обычно не большие (+ час-два к задаче). Еще, например, у нас сейчас в команде договоренность, что задачи в статусе ревью висят не более 24 часов (1-2 дня условно) - то есть задержка релиза тоже небольшая. Если задача маленькая, то ревью вообще практически не добавляет затрат - полчаса или меньше на ревью.
1-2 дня - это максимум на ожидание ревью. Обычно в тот же день или на следующий ревью проходит. Вы описали прям суровый вариант, когда что-то сильно пошло не так (причем еще до ревью), что аж несколько раз пришлось править замечания. В реальности я даже такого не припомню, обычно одного раунда правок достаточно, максимум двух.
Если задача очень большая, то перед тем, как ее делать, также можно обсудить какие-то архитектурные моменты в команде, чтобы как раз не править потом глобально после ревью уже готовую задачу.
Ну и также в момент разработки тоже можно советоваться с коллегами по каким-то вопросам, чтобы не возникало потом каких-то больших переделок на ревью.
Самые большие ревью с большим количеством правок возникают обычно в ситуациях:
в команду пришел новый человек и это его первая задача (и возможно она была сразу большая)
задача была большая, плохо декомпозирована и возникла проблема в реализации, требуется много правок
Если такое происходит, то главное сделать выводы из ситуаций и в будущем скорректировать процессы, чтобы их избежать. Например, давать новым сотрудникам задачи поменьше, обсуждать реализацию до начала разработки задачи (общие моменты: вроде "вот тут добавляем поля в БД, здесь делаем такие-то классы, вот этот функционал будет реализован аналогично такому-то"), дробить задачи на более мелкие.
Ну и еще вариант - если задача сделана, всё работает, а задачу очень ждут и времени мало, то замечания по ревью (если они не критичные) можно вынести в отдельную задачу с техдолгом и взять ее в следующий спринт (если у вас спринты).
а именно — навязывание ревьювером решения на основе его личного видения задачи
поясню
у нас в одной конторе было ревью, всё как в статье, все смотрели код друг друга, писали замечания улучшали качество и т.п.
но у нас был один товарищ (чуть выше по иерархии остальных), который считал что его точка зрения на реализацию любого сервиса гораздо важнее, всегда отписывал на каждом ревью огромные комменты, вплоть до того что 'а вот я тут в своей веточке все сам сделал, у вас както плохо вышло'
причем он был везде в списке аппруверов и насмотртя на правило 'мы даем только рекоммендации если не нравится реализация но она работает', он никогда не ставил аппрув если чтото ему не нравилось
в итоге вся комманда писала код так чтобы этот единственный чел был доволен.
и еще разок сталкивался с подобным человеком уже в другом месте, он сильно придирался к форматированию кода и тоже постоянно тянул ревью по времени… буквально пару часов в комментах можно было спорить по поводу очередности блоков кода
Про это как раз написано в месте - что все ревьювят всех. Я считаю, что не должно быть такого, что кто-то один в команде принимает все решения. Вы правы - это бывает очень токсично и непродуктивно + вредит продукту скорее всего. Команда все-таки должна уметь договариваться и уважать мнения друг друга. Если есть аргументы, почему какое-то решение человек считает подходящим, то можно донести это при ревью и ревьювер может пропускать код, даже если был изначально не согласен с чем-то. Спасибо, что вынесли этот кейс в комментарий.
Заметка справедлива, как мне кажется, для однотипных решений в коде и боле-менее похожих задач. И когда программистов много. Когда программистов мало и они делают абсолютно разные проекты (иногда в разных ЯП), то затраты времени на вникание в код нерационально велики. В таком случае можно договориться о каких-то правилах, основных именах, функциях, для возможной поддержки кода не автором, но не проверять всё. Просто вводится персональная ответственность автора. Подозрительные моменты можно найти при проверке чужого кода, но так ли код работает, как надо по ТЗ - выяснится только на тестовых запусках программы.
Опишите ваш опыт, потому что я не понимаю, как это делать в малой команде.
Мне кажется, что в небольшой команде наоборот легче всего ревьювить. И быстрее всего. Вот когда продукт разделен на разные части и там разные команды отвечают за код, а вы отдаете в ревью что-то другой команде - то вот тут могут быть случаи, что ревью зависло и надо отдельно напоминать про него другой команде. Уже второй комментарий про опыт, наверное действительно стоит сделать отдельно статью про реализацию)
Как вы боретесь с просьбами в личке "заапрусь позязя, релиз горит, PO копытом землю бьет"? Это самая уродливая часть всех code review поголовно, когда остается такая брешь для человеческого фактора и начинаются подковерные игры. "Ноет в личке, но отказать нехорошо, ведь через пару дней на его месте буду я" или "о, мудила тянул мой PR до последнего, пусть теперь подождет, проникнется". Как быть, если я вообще не хочу ни с кем обсуждать пулл-реквесты в привате, потому что для меня это рабочий процесс, а не предмет личной услуги?
У нас такого, к счастью, нет) Мы в целом договорились что ревью за 1-2 дня проходит.
Но я сталкивалась раньше с такими случаями. Мы решали такие вопросы с тимлидами и в целом с четким воркфлоу в командах. Например, у нас были случаи, когда другая команда вносила правки в код нашей команды в рамках их задачи и ревью было на нас - мы договорились, что у таких ревью приоритет выше, чем у всех остальных ревью и старались их делать быстрее. Ну и в обратную сторону - аналогично. Другие команды старались наши ревью смотреть быстрее и приоритетнее.
Вообще ситуация "срочное ревью" возможна только если это багфикс чего-то на бою и обычно там не очень много кода и ревью можно быстро провести. В остальных случаях по идее не должно быть такой срочности. Если она есть, то, по-моему, тут что-то не так с процессами, возможно, на уровне приоритетов задач от PM.
Ну и в игры "тянул мой PR, я тоже потяну" - для начала просто не вступайте, это непродуктивно и никому от этого хорошо не будет. В целом это вопрос про культуру в командах. Если есть возможность, можно это обсудить на ретроспективах или донести до тимлидов команд. Чтобы был процесс, который все понимают и не было необходимости отвлекать кого-то в личке. Ну и посмотреть, почему вообще с какими-то задачами возникает такая срочность именно на этапе ревью и действительно ли она возникает или это просто разработчик хочет быстрее закрыть тикет, а то такое тоже бывает.
Прекрасный гайд по проведению код ревью: https://google.github.io/eng-practices/review/
Интересные вопросы здесь в комментариях, аж захотелось написать статью с разбором этих вопросов.
У нас в компании очень серьезный подход к код ревью, они обязательные и двухуровневые. Если интересно – поделюсь опытом. Пишите наболевшие вопросы в комментариях, это будет практичнее.
Большое спасибо за статью, информация очень хорошо изложена. Данная тема очень актуальна.
Спасибо за статью! Как по мне, код ревью это замечательный процесс, если он налажен и отработан в команде, и все стараются выполнить его флоу
Согласна с тем, что Code Review ничего не гарантирует, но не согласна, что его не стоит использовать. По-моему, плюсов гораздо больше, чем минусов.
Насчет почвы для конфликтов – это опять же про культуру в команде и soft skills. Если люди не могут договориться, не могут корректно давать обратную связь и воспринимать ее, то тут проблема не в Code Review.
Видео посмотрю, спасибо за ссылки!
Code Review – зачем и как использовать в команде?