Code Review, Windows style

Для полноты буду дописывать про практики в MS. Code Review есть почти во всех группах, о которых я слышал, но в разных более или менее тщательный. Вы значит извините, если я буду банальности говорить.

Где пишут Windows – один из наиболее жестких. Наша графика таки не совсем глубокий kernel, может там еще хуже.

Так вот, Code Review обязателен на каждый чекин в source control. Баг, фича, рефактор, что угодно – никогда нельзя положить без CR. Хуже, надо чтобы посмотрели как минимум два человека, из которых как минимум один – девелопер в твоей команде.

Как это примерно выглядит.

Есть тулза, которая умеет превращать diff’ы открытых файлов в patch file с изменениями. Потом этот патч можно применить на другой машине, просто посмотреть какие в нем изменения (причем посмотреть вместе со всем кодом измененных файлов), замерджить их со своими и так далее.

После того как ты починил баг/написал фичу/whatever делаешь такой патч, выкладываешь его на public share и пишешь письмо людям, от которых хочешь получить ревью. Типично это твоя команда, если фича затрагивает другую команду – владелец задетого куска в той команде. В особо веселых случаях – security/performance team.

На это письмо тебе отвечают всяческий фидбек, ты объясняешь и/или фиксишь что-то в коде, выкладываешь новый патч (тулза умеет показывать diff между двумя паками), и так несколько итераций, пока каждый ревьюер не скажет “looks good”.

После этого заливаешь изменения в source control, помечая в комментах имена тех, кто делал Code Review. Помечать имена стимулирует делать хороший CR – если у изменений, которые ты ревьюил, много регрессий, то это будет заметно.

Совершенно без откровений, все абсолютно по книжке.
Ну и самое главное – не очень важно, какой конкретно технически процесс. Важно, чтобы CR был и работал для вашей команды. Будете вы там обмениваться паками от diff или ходить в чужие бранчи – дело второе. Важно, чтобы CR был и делать его было удобно.

Чего мне хотелось бы отметить кроме этого.

Во-первых, CR безумно полезен.
Банальность, но стоит повторить. Очень часто находят ошибки сразу, причем часто – очень простые, но какие тестированием находить тяжело. Что забыл сделать отдельный error handling, что забыл освободить ресурс и так далее. Скажем, что случайно поставил y, а не x. “Забыл” и “случайно” – ключевые слова. Не надо предаваться фантазиям о том, как заставить проверять x и y компилятор, надо делать Code Review.

Это в случае, если ошибки в изменении мелкие. Если CR найдет крупные, будет еще полезнее. К чужому коду изначально относишься подозрительно, и это бонус.

Еще один бонус – обмениваться опытом и вообще расширять знание кода. Смотришь как пишут код другие люди, сравниваешь с тем, как бы написал сам. Если лучше – пишешь фидбек, если хуже – радуешься. Постоянно глядишь в чужие части кода, больше про них понимаешь. В случае чего, будет гораздо проще разобраться.
Вообще, конечно, лучший способ разбираться в чужом коде – это фиксить в нем баги, но Code Review тоже помогает.
Можно более свободно давать людям копаться в твоем коде, ты все равно посмотришь на их изменения прежде чем они попадут в код.

Ну и напоследок – это хорошие гарантии того, что совсем плохой код не попадет в кодобазу. С Code Review можно садить новичков фиксить баги и не бояться ужасов. Если код зачекинили, ты сам его пропустил. Да, надо больше и внимательнее смотреть на CR у новых людей, но это гораздо лучше неконтролируемого доступа.

Как показывает практика, разные люди в команде смотрят на разное. Кто-то пристально следит за соблюдением Code Standard, кто-то перформанс фрик, кто-то докапывается до деталей корректности. Это все хорошо и здорово.

Собственно, когда пишется важный и тонкий код, с большими требованиями по security/performance/complexity – вопрос даже и не стоит, без Code Review никак.

Во-вторых, CR совсем не бесплатен.
Время от написания фикса до его попадания в source control – увеличивается на день или несколько дней, в зависимости от тщательности code review. Что интересно, как мне кажется, в основном увеличивается latency, а не bandwidth (да, да, и тут они). То есть, ты отослал код на ревью, его будут еще как минимум полдня читать, потом напишут фидбек, ты поправишь, его еще раз уже побыстрее прочитают и т.д. – latency большое. С другой стороны, ты в это время работаешь над другими багами. Время делать сам CR посреди дня обычно находится – хорошее занятие после обеда, хорошее занятие после чая и вообще в моменты перерыва фокуса. То есть, сам по себе CR даже у нас не жрет больше 10-20% времени, но так как в случайное время посреди дня – то вот такая задержка.
К сожалению, иметь много багов “в полете” тоже плохо – нельзя часто переключаться. Остается искать разумный баланс. Для меня сносное число – 3-4 бага или одна фича в полете, это в принципе примерно равно количеству перерывов в рабочем дне :) Говорят, со временем получается лучше, я чего-то сомневаюсь.

Подытожив – расходов фактических можно оценивать в процентов 20, время до чекина – увеличивается на дни.
Как обычно, latency нельзя недооценивать.

В некотором смысле, CR – разумная альтернатива pair programming, которая при этом гораздо дешевле.

В-третьих, нравятся мне эти файлы с патчами.
Надо что-то слить такое забавное для остроты… Ну вот наша тулза называется bbpack и ее написал сам Реймонд Чен на (барабанная дробь) Перле. В других местах MS тулза какая-то другая.
Так вот, бибипаки клевые. Например, можно двум девелоперам делать взаимно зависимые изменения, никогда не ломая билд. Один делает изменения, отсылает другому патч, тот его у себя применяет, делает свои изменения и заливает все за один раз. Или можно отдать тестеру локальные изменения, чтобы он их применил и проверил. И еще всякие применения.

Это, опять же, все совершенно не откровения. Насколько я понимаю, в мире open source обмениваются такими же патчами diff’ов. Это радует, потому что значит такая тулза делается самому на раз.

Вот примерно так, отчитался.
UPD: У Гугля, кстати, тоже хороший подход.

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

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

Но если вы обсуждали дизайн не детально (а так чаще всего и бывает), может, стоит глянуть на первые чекины с конкретными классами? Если есть тонкий архитектурный момент, если perf-sensitive кусок, если много разбросанных изменений, если залезли в чужой код – может пусть на всякий случай кто-то глянет?

  • IronPeter

    diff – да, полезный тул. Уксовый стандартный. Тут говорят http://ru.wikipedia.org/wiki/Diff что окончательная версия в 74 году устаканилась. Классико!

  • http://unigine.com/ binstream

    Как в винде разработчики живут без diff и patch – действительно сложно представить. Кстати, наверняка с каким-нить cygwin можно все это счастье поиметь и под виндой “из коробки”, ничего не дописывая.

  • Dimchansky

    diff & patch для windows существуют в пакете gnuwin32:
    http://gnuwin32.sourceforge.net/packages/diffutils.htm

  • Viacheslav

    Code review да и вообще “процесс” – это очень полезно. На моих глазах пример работы на Motorola. Code review обязателен, вообще тулза автоматически следила, что без правильно заполненной инспекции ни один build request не может быть подан, даже в нашу компонентную метку. А в сочетании с unit test + feature test + regression test вообще давало на мой взгляд отличные результаты, хотя и затягивала процесс в разы. Тут главное побороть сопротивление нижних слоев программистов, с горящими глазами пытающихся выдавать на гора сотни строк кода в день. Но, собственно, и цена ошибки у нас сильно выше была, чем, например, в “обычном российском PC-шном геймдеве”, где мне тоже представилась возможность поработать. Т.е. качество кода даже сравнивать нельзя, при том, что уровень программистов сильно не различался.
    Вопрос в том, применим ли такой процесс, например CMM5, для PC-шного геймдева – не знаю.

  • shodan

    binstream, ну почему же без? ;) http://unxutils.sourceforge.net/

  • nol

    Хорошо написано. В гугле люди пошли дальше, их Mondrian (http://video.google.com/videoplay?docid=-8502904076440714866 – видео о нем) написал Guido van Rossum (атец питона). Жалко он внутренний.

    Диффы и паки с ними отдельная тема. ИМХО, делать совсем закрытым VCS не очень хорошо. Кажется, что правильнее сделать закрытой ту его часть из которой собирается продукт. Во-первых, это позволяет ревьювить изменения и изменения к изменениям с возможностью сбилдить и посмотреть отладчиком если что-то нетривиальное смущает. Во-вторых, эти самые диффы имеют свойство терять актуальность по мере чекинов затрагивающих те же файлы что переносит заботы соурс контрола на плечи людей. В-третьих, совмещать чужие изменения со своими перед чекином в главную ветку становится легче, а в некоторых VCS(p4) еще и поддерживается. В-четвертых позволяет в процессе написания/отладки чекинить промежуточные варианты для истории и большего удобства КР “по частям”. В-пятых, на такие вот ветки можно легко натравливать всякие автоматические системы сборки, юнит-тестирования и чего еще душе захочется. Latency при этом к сожалению никуда не девается…

  • CEMEH

    binstream, вопрос исключительно желания, как я понимаю, тот же WinDiff это умеет. Разумеется, если делать себе такое – надо пользоваться diff/patch. Дописывать именно интеграцию с source control (быстро сделать patch из открытых файлов, замерджить патч в другой бранч и т.д.)

    nol, я слышал в других командах хотели ввести нечто подобное той системе Гугля. У нас конкретно не такой большой изолированный кусок кода, поэтому вполне справляемся письмами с паками.
    Разумеется, для разных частей source control разные политики. В свои конфиги – чекинь что хочешь, на тестовый код – гораздо меньше ограничений и т.д.
    То что диффы теряют актуальность – этого как бы не миновать. Разумеется, source control должен поддерживать merge и т.д. В этом смысле, у наших паков есть приятная особенность – кроме диффов, они сохраняют и изначальный файл, то есть ты всегда можешь посмотреть не просто изменения, а “Before/after”. То есть, если даже файл очень сильно изменился, можно посмотреть что собственно было накодано, и если что более вменяемо перенести.
    Чекины в процессе отладки вполне можно делать в свой личный Private branch. А вот при интеграции в основной – Code Review.

  • nol

    Достоинство тулзы гугля в том что комментарии, код, история ревью – все в одном месте и всегда доступно, удобно и функционально.
    Вот посылать что-то по почте меня честно смущает.
    Я говорил о подходе, когда разработчики чекинят в свой бранч все свои изменения. Потом, в процессе интегрейта, эти изменения и будут составлять дифф который предназначен для ревью. Заметим что этот дифф “живой” – т.е. сделав его через 3 дня после запроса на ревью я уже буду видеть эти изменения на фоне актуального состояния кода, т.е. конечный результат чекина. Дополнительный плюс: реакция раработчика на комментарии КР это тоже чекин, который, как правило, гораздо меньше оригинала что облегчает повторное КР и, в итоге, ускоряет (ха-ха, неужели уменьшается Latency!) чекин в основной бранч.

  • CEMEH

    В MS очень много через почту, прямо очень много. В день в среднем приходят десятки писем, большинство дискуссий, новостей и рабочего – там. Что считаю тоже очень хорошая практика, стимулирует и детально отвечать, и есть архив обсуждений решений. В такой культуре CR по почте вполне живет.

  • http://users.livejournal.com/_zerg/ _zerg

    под виндой diff вполне живет через svn. есть ли патчи в свн не знаю, тогда было как то не до того, а в последних конторах юзали другие тулы.

    а статья отличная!

  • http://devlog.gw-labs.net/ dorfe

    CEMEH:
    > В MS очень много через почту, прямо очень много. В день в среднем приходят десятки писем

    Немного оффтоп, но все же: какой почтовик юзают в MS – Office Outlook или все же Windows Mail (бывший Outlook Express)?.. Если второе (в чем я лично сомневаюсь), то юзаете/слышали ли о Fidolook (www.fidolook.org)?.. ;-)

    И что вам известно об этой истории: http://www.fidolook.org/flhelp/work/article/history/

    :)

  • CEMEH

    Полный Аутлук, разумеется. Про эту историю лично мне – ваще ничего не известно :)