Pull to refresh

Comments 35

Ситуация: разработчик добавил в проект три файла и отправляет их на инспекцию коллегам, которые находят несколько дефектов.


Если не секрет, откуда взяли такой… use case?
Вполне такой жизненный use case, чем Вам не нравится?
Мне он всем нравится. Хочу понять, откуда в жизни берутся такие юз кейсы и какова их доля от всех остальных.
Непосредственно у вас так делают?
Юзкейз такой появляется при введении в процесс разработки Code Review. Или Вы хотите узнать зачем вообще нужен Code Review?

Лично я его видел неоднократно, у нас в отделе хотим в ближайшее время ввести.
Я уже много лет занимаюсь руководством разработкой и code review у меня есть. Более того, я его делал несколькими разными способавми. И есть у меня ряд вопросов по данному юз кейсу — но задавать их имеет смысл тому, кто это делает :). Иначе будет сферическое обсуждение непонятно чего в вакууме :). Так что очень надеюсь, что автор оригинального поста снизойдет до старика и мне ответит ^_^".
Насколько я это видел, такой юзкейс возникает при наличии стажёра или просто новичка в проекте.
Есть у меня подозрение, что автор рассматривает code review чуть пошире, чем инструмент воспитания новичков. Обучение новичков и стажеров — дело, конечно, очень нужное и важное — но, ИМХО, это не тот бизнес процесс, ради которого стоит такие огороды городить. Хочу общаться с автором -_-. А автор со мной явно общаться не хочет — у них компания серьезная — ROI там, TCO, не до вконтактиков и социалочек :(.
Я говорил именно про указанный юзкейс. =)
Про более широкий смысл кодревью я догадываюсь )
Указанный кейс не очень интересен — ИМХО, обучение новичков разработке и непосредственно разработка — это вещи разного порядка. Обучение — отдельная тема, там много разных подходов, инструментов и методик. Лично меня как руководителя разработкой более интересует применение code review как раз при разработке.

С другой стороны, про обучение тоже есть что сказать. Вы лично такой юз кейс при обучении новичков применяли?
>>откуда в жизни берутся такие юз кейсы
Я не ТС, но давайте подумаем вместе: вы код проекта на чем пишите? Думаю, что на компьютере. Исходный код программы состоит из файликов. Имеет смысл разделять исходный код программы на компоненты и компоненты раскладывать по файлам исходных кодов. Когда соответственно создается что-то новое, то на начальном этапе количество файликов равно 0 и при разработке увеличивается. Отюсда появляются такие клевые юзкейсы, как описанный автором.
Когда соответственно создается что-то новое, то на начальном этапе количество файликов равно 0 и при разработке увеличивается. Отюсда появляются такие клевые юзкейсы, как описанный автором.


Потом количество файлов очень быстро достигает порога насыщения и дальнейшие изменения идут уже в самих файлах, без добавления новых. Основной показанный юз кейс — на первые три дня развития проекта? Все последующие месяцы code review можно не использовать, потому как новых файлов почти не добавляется? :)
Я вас понял. Перечитав немного стать, я думаю, что имеется ввиду процесс добавления файлов в Ревью, а не в проект. Т.е. вот я зафиксал багу, в этой баге я поменял файлы 1.txt и 2.txt. Вот эти файлы будут добавлены в ревью.

Ничего за возможность ревьюить или подстветить чейнджи в этом фиксе не скажу, но имхо вещь необходимая.
А дальше мы плавно переходим к следующему впрос — почему мы хотим ревьювть коммит? В соответствии с актуальной теорией разработки софта, коммиты должны быть маленькими. Чем меньше — тем лучше. Соответственно, любое нетривиальное изменение кода (а мы ведь хотим ревьювить не только однострочные багфиксы?) — это десятки коммитов в рамках выполненной задачи. И вот тут совершенно непонятно, что нам предлагает автор и описанная тулза. Аплоадить эти коммиты по одному чтоли? Так они по одному бессмысленны — они исключительно в совокупности могут быть оценены, как «последовательный набор изменений в рамках решения указанной задачи».
Какие странные теории.

Да, комиты должны быть маленькими, но это не значит что они должны быть однострочными, это более вероятно означает, что они должны содержать оформленный кусок чего-то одного. В оптимальном варианте — это задача/баг. Если задача слишком большая, она стопудово бьется на подзадачи.

Когда выработаете с User Story, то разбивая для правильного менеджмента их на таски, хорошим способом является разбивать их на таски не более 8-13 часов, иначе разработка данной юзерстори становится труднее для отслеживания. За 8 часов много не напрограммируешь если делать это правильно.

Когда мы говорим о хранении кода в VCS, то в центральном репозитории всегда хранятся завершенные куски, т.е. закомиченные таски. Т.е. там дибилятника в роде построчеченого комита каждого класса быть не может, иначе никогда не найдешь и не откатишь.

В случае распредленного VCS, при наличии локального репозитория — да, делаете мелкие комититы, но опять же, в стиле завершенных мыслей. Но вы же не будете предоставлять на ревью завершенную мысль, а не завершенный таск — это было бы неэффективно с точки зрения траты времени ревьюера.
Но вы же не будете предоставлять на ревью завершенную мысль, а не завершенный таск — это было бы неэффективно с точки зрения траты времени ревьюера.


Не понял. То есть вы согласны с тем, что если задача не решается в один коммит (а это большинство задач в реальной жизни при разработке софта сложнее hello world), то вариант «добавить в ревью файлы» не работает вообще, потому что ревьювить надо работу целиком, а там нет «файлов» как таковых — там два десятка коммитов?
Во-первых, я не считаю что в задаче может быть 2 десятка коммитов. Мое твердое убеждение, что 5 — это максимум того что может быть в центральном репозитории, и это если разделилась на подзадачи.

Во-вторых, даже если скажем там n-комитов, то в при использовании описанного тула я бы делал так:
— посмотрел какие файлы поменяны в ревизиях [x1, x2, x3-xM, ..., xY] (те которые относятся к комитам по задаче)
— все эти файлы добавил на ревью/в ревью таску.

Но самое главное, по чему я бы скучал: я склоняюсь к мнению, что неотревьюиные таски не должны бы лежать в центральном репо. Поэтому, было бы прикольно иметь некий промежуточный центральный репо, или его прокси, с интерфейсом для код ревью тула, где можно было бы жать кнопку типа «Review done» и это все переносилось бы в центральный репо от имени автора.
Во-вторых, даже если скажем там n-комитов, то в при использовании описанного тула я бы делал так:
— посмотрел какие файлы поменяны в ревизиях [x1, x2, x3-xM, ..., xY] (те которые относятся к комитам по задаче)
— все эти файлы добавил на ревью/в ревью таску.


Разумно. Но, даже если отбросить факт необходимости ручной работы на каждый чих — Интрига: описанная программа так не может :). Она детектирует «измененные файлы» по состоянию «uncommited» в VCS (тоесть просто запускает соответствующую тулзу VCS с аргументом diff и смотрит что изменено и не покоммитено).

Вот мы и сформировали основной вопрос автору — а как это планируется использовать в реальной жизни, а не на приведенных примерах hello world? :)
Она детектирует «измененные файлы» по состоянию «uncommited» в VCS

Если кстати VCS — git и состояние uncommitted смотрится в локальном репо (что по анологии с языком гита будет unpushed), то можно тем самым сэмулировать этот «прокси репозиторий». Тогда можно получить полный список непокомиченных файлов по таске. Это я так гадаю.
Это вы что-то новое о git рассказали O_O. «git diff» показывает изменения, которые на данный момент не закоммичены в локальный репозиторий. Каким образом вы собрались смотреть диффы всего, что закоммичено и не закоммичено, но еще не запушено? O_O

В любом случае, важной частью коммита является его комментарий, без него делать review мало осмысленно, даже если собрать все коммиты в мгучую кучку. Так что чит не пройдет. Тулза не может :).

… Или я что-то о ней не знаю. Все еще надеюсь что автор ответит :(.
Каким образом вы собрались смотреть диффы всего, что закоммичено и не закоммичено, но еще не запушено? O_O

Я собрался смотреть дифы закомиченного в локальный репо, но еще незапушеного в центральный.
Это много команд подряд. CodeCollaborator так не может.

BTW, даже если и сможет — это тоже бесполезно :). Потому как на практике разработчик часто работает одновременно более, чем над одной задачей. Я же тут не просто так полемику развожу — много лет разработкой руковожу, code review — больная тема, вопросов море, как делать правильно никто не знает и все такое прочее.
Это много команд подряд.

Как мне рассказывали, там немного.

Потому как на практике разработчик часто работает одновременно более, чем над одной задачей

Ребята в кедах и на маках расскажут вам, что под каждую задачу создается ветка в гите. Поэтому опеределить поменяные файлы в ветке — не есть сложная задача.

CodeCollaborator так не может.

Этот тул можно допилить. Кстати, где он? Нам просто похвастались?
По поводу просмотра незапушенного плюсую — не знал о такой функциональности, полезно.

По поводу допилить — он не open source, а вполне себе коммерческий продукт. Его можно только локально для себя допиливать. Это дорого.

Кстати, где он? Нам просто похвастались?


Кто, CodeCollaborator? Там же где и всегда, у умных мишек. Этой тулзе уже много лет, она в целом давно и прочно в своей нише живет. Другой вопрос что я в упор не понимаю что это за ниша такая для детского сада и очень хочу поспрашивать автора как это на практике выглядит. Но — видимо не судьба :(.
А, я че-то думал, он внутренняя поделка.

Короче, имхо, вид код-ревью процесса в том техническом исполнении оптимален, в котором я его описал на протяжении нашей дискуссии и тут:
Но самое главное, по чему я бы скучал: я склоняюсь к мнению, что неотревьюиные таски не должны бы лежать в центральном репо. Поэтому, было бы прикольно иметь некий промежуточный центральный репо, или его прокси, с интерфейсом для код ревью тула, где можно было бы жать кнопку типа «Review done» и это все переносилось бы в центральный репо от имени автора.

Вроде как crucible такое позволяет делать через патчи, но это несколько далеко от идеала.
Вроде как crucible такое позволяет делать через патчи, но это несколько далеко от идеала.


Ну это «с помощью напильника сделать из трактра самолет». Сам Crucible пропагандирует практически тот же подход, «давайте отправим файл на ревью». Насколько я знаю по инсайдерской информации, сами авторы crucible не пользуются по причине «несовместимости с реальной работой», а там где пользуются оно феерично замедляет разработку и роняет качество кода ниже плинтуса — коммиты становятся большими, толстыми — что приводит к технической невозможности ревьювить их содержимое :).
По поводу просмотра незапушенного плюсую — не знал о такой функциональности, полезно.

По поводу допилить — он не open source, а вполне себе коммерческий продукт. Его можно только локально для себя допиливать. Это дорого.

Кстати, где он? Нам просто похвастались?


Кто, CodeCollaborator? Там же где и всегда, у умных мишек. Этой тулзе уже много лет, она в целом давно и прочно в своей нише живет. Другой вопрос что я в упор не понимаю что это за ниша такая для детского сада и очень хочу поспрашивать автора как это на практике выглядит. Но — видимо не судьба :(.
Извините меня за долгий ответ, был в командировке с четверга.

Более подробно формализации use-case'ов инспекций кода у нас была посвящена предыдущая статья. В ней рассмотрены, в частности, межкомандные инспекции.

Этот пример я взял из личного опыта предыдущего проекта, и мне было просто удобно на нём продемонстрировать интерфейс CC.

На предыдущем проекте у меня часто были задачи поддержки какого-то нового оборудования. Например, нужно поддержать новый контроллер или прибор учёта какого-то производителя (например). В этом случае создавался каталог для этого счётчика и создавались файлы реализации логики конкретно этого прибора, которые потом попадали на инспекцию (мы вели разработку прямо в trunk’e). Мы создали очень хорошую объектную модель(что типа фреймворкa), у которой всё было разнесено по разным уровням абстракции, и для поддержки нового прибора чаще всего нужно было создать реализацию абстракций именно под этот конкретный прибор. Схема очень похожа на модель драйверов в ядре Linux'a. Там есть так называемый bus драйвер, ниже — драйвер контролера, ещё ниже — драйвер конкретной железяки. Добавление достаточно объёмных новых файлов у нас было нормальным явлением.

Заводилась одна большая задача, которая делилась на несколько небольших подзадач, по каждой подзадаче делались commit'ы и на каждую подзадачу создавалась отдельная инспекция. Деление проводилось по разным логическим единицам: поддержка чтения архивов, поддержка чтения журналов, поддержка чтения текущих значений и т.д.
Т.е. разработчик каждый день, когда хотел, коммитил свой код в репозиторий. Затем, когда приходило время инспекции, указывал все нужные ревизии в инспекцию.

Когда я инспектирую вновь добавленные файлы, в которые уже было сделано по несколько commit'ов, то хочу видеть последнее состояние файла без промежуточных изменений, которые для меня не интересны. Вот это как раз позволяет сделать СС автоматически.
то хочу видеть последнее состояние файла без промежуточных изменений, которые для меня не интересны


Любопытная позиция. Правильно ли я понимаю, что при этом теряются комментарии ко всем «промежуточным» коммитам, ревьювер их не видит? Вы считаете комментарии к коммитам не интересными?
Уточню разбираемую мной ситуацию, дабы убедиться, что мы говорим об одном и том же.

Разработчик работает над фичей недельку и каждый день делает commit’ы, дабы не заморачиваться с backup’ами. Допустим, что у него получилось порядка 15 commit’ов в разные файлы и что один файл участвовал во всех 15 commit’ах, причём код в этом файле правился, добавлялся, удалялся, рефакторился.

После того, как разработчик посчитал, что закончил работу над функционалом, и готов представить результаты на инспекцию, он создаёт инспекцию и добавляет все ревизии, которые относятся к этой задаче.

Меня как инспектора интересует diff было-стало. Что там было посередине – для меня не интересно, слишком дорого.

Лично мы своей командой работали в trunk’e, но на каждую задачу создавали свою рабочую копию. Поэтому, когда коммитил изменения по одной задаче и переключался на другую, то всегда обновлял рабочую копию, чтобы все изменения объединились.

Если работая над задачей видел, что какой-то разрабатываемый функционал будет нужен мне в других задачах, то мог его разработку провести отдельной подзадачей. Такое, правда, случалось очень редко, т.к. в большинстве своём задачи в работе (как правило, не больше 3 задач) редко пересекаются кодовой базой. Поэтому мы решили, что такие единичные случаи нам дешевле обработать, проведя разработку общей части в рамках новой подзадачи (прямо в багтрекере разработчик заводил новую задачу, в рамках которой выполнялась разработка общего функционала).

Поэтому, когда я начинал инспекцию, мне было совсем не интересно, как там правил код автор, и пытаться понять, каким образом автор пришёл к конечному решению — слишком дорого, в моём понимании.

Мы проводили инспекцию итогового решения:
1) удовлетворяет ли оно предъявляемым требованиям;
2) достаточно ли use-case’ов покрывают unit-тесты;
3) проверить код на отсутствие логических дефектов (например, конфигурация прибора читается до начала работы с ним);
4) проверить на применение правильных идиом (те же shared_ptr, отсутствие утечки ресурсов и применение RAII);
5) проверить корректную обработку граничных условий и обработку и сигнализацию ошибок (exception/return value).

Резюмируя, не припомню, чтобы комментарии к коммитам как-то помогали мне на инспекциях. Unit-тесты и комментарии к ним – да, doxygen комментарии к функциям/классам – да, комментарии в коммитах – нет.

Имхо, конечно.
Спасибо за комментарии. Use case стал гораздо более понятным — по материалам статьи было неочевидно, что code collaborator способен работать не только с текущими незакомиченными изменениями, но и с набором revisions. Это действительно позволяет его использовать в коммерческой разработке, а не только для hello wold :).
Что могу рассказать по существу. Кроме очевидных достоинств, у описанного способа code review есть ряд недостатков:

1. Непонятно как премнять, если продолжительность задачи более одного дня. Тоесть в теории менеджмента мы должны все задачи бить на куски не более одного дня — но на практике именно программирования часто минимальный, неделимы кусок работы занимает от нескольких дней до пары недель. Если при этом code review проводить в конце, «когда все готово» — то можно очень лего словить ситуацию что все придется переделывать нафиг, потому что разработчик в начале / середине допустил фатальные архитектурные ляпы. Стоять за спиной человеа не всегда удобно — особенно если он в соседнем офисе или другой стране — это популярно. Вручную изучать его коммиты по мере их создания — чистой воды дублирование работы. В случае возникновения продолжительных зада гуру рекомендуют вместо review (о коммита, условно) использовать audit — ревью кода в процессе написания. Но насколько я понимаю, Code Collaborator такой подход вообще не поддерживает, все ревью исходят исключительно от разработчика и нельзя просто взять и отревьювить несколько произвольных коммитов в разумное количество кликов? Или можно?

2. Если коммиты делать мелкими (а коммиты [b]нужно[/b] делать мелкими, потому что коммит — это нифига не разновидность бэкпа, это важная информация о коде — комментарий коммита и небольшие изолированные коммиты позволяют получть о коде гораздо больше информации, чем средний программист способен засунуть в комментарии и именования идентификаторов) — то на типичную задачу среднего диаметра их может быть пара десятков. Соответственно, если доверить програмистам вручную составлять список коммитов, то часто ловится ситуация когда что-то не добавили / добавили не то — в результате тратится огромное количество времени на синхронизацию. Обычно гуру рекомендуют в такой ситуации в цоментарии к коммиту прописыать идентификатор задачи из Jira, чтобы затем можно было легко найти все коммиты, оносящиеся к выполнению данной задачи. Так может делать crucible, Jira'вская часть для Code Review. Может ли так делать Code Collaborator?
Да, недостатки действительно есть.

По-моему, это как с «техническим долгом»: какой можно принять, чтобы в краткосрочной и долгосрочной перспективе заработать больше, чем без него.

По-существу.

1.1 Да, вы можете создать инспекцию, добавлять туда комиты по мере их появления и просматривать: добавился новый комит в инспекцию, вам пришло уведомление, посмотрели, прокомментировали. После того, как вы описали проблему, вспомнил, что мы так действительно поступали с большими задачами :-)

1.2 Самый лучший способ, который обеспечивал нас хорошими архитектурными решениями – Design Review. Перед началом того, что я хочу сделать, накидывал прототип, определял основные концепции и выкатывал это своим коллегам, которые критиковали и показывали узкие места. Таким образом, до начала реализации в рабочей системе мы тратили ~10% на прототипирование, зато получали отличные качественные архитектурные решения. CodeReview здесь уже помогает лечить проблему, но не причину. Предназначение же Design Review – лечить причину на корню. Думаю, что самый отличный вариант гармонично совмещать Design Review и CodeReview. Уточню, что Design Review проводили для действительно стоящих новых задач/рефакторинга, для «мелочи» этого не делали.

2. Да, может. Для этого настраивается так называемый Automatic Link – по сути это регэксп, найдя который в текстовых полях шапки инспекции, CC подставляет для него шаблон подстановки. Например, вот такой паттерн #(\d+) превращается у нас в ссылку в TestTrack при помощи такого шаблона ttstudio:///MaxPatrol8/dfct?number=$1 (#11001 -> ttstudio:///MaxPatrol8/dfct?number=11001)
С Джирой можно так же – это точно, на предыдущей работе у нас были ссылки в Джиру. Мы именовали инспекции, просто вводя код запроса из Джиры. Очень удобно. Всё равно краткая инфа в списке инспекций мне ничего не даёт, а вот ссылка на Джиру очень даже хороша. Тем более что есть функция сортировки по полю, так любую инспекцию легко найти.

Сейчас хотим добавлять ссылку в запросе bugtracker’а на инспекцию, чтобы было удобней. Пока что программист в запрос TestTrack вручную будет добавлять ссылку на инспекцию в CC.
Еще вопрос к автору: а как ведет себя CC, если патч для SVN содержит перемещения файлов, пакетов и т.д.? Ему голову не сносит? Просто сталкивался, что другие инструменты иногда ведут себя в таком случае некорректно.
Такой живой интерес к посту не может не радовать :-)

К сожалению, всю прошлую пятницу хабр оставался для нас недоступен. Видимо, корпоративный IP был зафильтрован в процессе масштабной борьбы с DDoS-атаками. Сейчас ситуация изменилась, и в ближайшее время предоставим вам автора ;-)

Еще раз, огромное спасибо за вопросы и комментарии!
CC не умеет отслеживать переименование/перемещение файлов в SVN. Запрос на разработку этого функционала висит еще с 2008 года. Разработчики отмазываются ограничениями самого SVN.

Переименование/перемещение файла CC понимает как удаление старого и создание нового. Не совсем удобно.

Хотя такие ситуации возникают достаточно редко. У нас были заведены, например, шаблоны именования файлов.

С другой стороны, если файл разрастается, то в конечном итоге, он подвергается рефакторингу и делению на несколько файлов. Корректность copy-paste’а из старого файла в новый приходится отслеживать вручную.
Sign up to leave a comment.