Пользователь
0,1
рейтинг
10 июля 2009 в 16:38

Разработка → Перевод: Я ненавижу тебя: твой код – хлам!

Хочу представить свой перевод статьи «Your Code Sucks and I Hate You: The Social Dynamics of Code Reviews».

Я ненавижу тебя: твой код – хлам!
Взаимоотношения участников ревизий кода

Джонатан Лэндж (Jonathan Lange), 15.09.2008

Обзор


Ревизия кода это действительно полезная, но в то же время и невероятно отпугивающая процедура. Эта статья подскажет, как избежать «кулачных боёв» при проведении ревизий.

Мы кратко рассмотрим, почему следует проводить ревизии кода, и сделаем упор на вопросе, как складываются при этом взаимоотношения участников процесса, в особенности в проектах с открытым исходным кодом. Действительно, отчасти open source привлекает (а порой наоборот отпугивает!) людей именно потому, что ваш код будут просматривать эксперты со всего земного шара. Мы также рассмотрим влияние, оказываемое некоторыми существующими технологиями на культуру ревизий кода, рассмотрим, чего можно достичь с их помощью, и как проводятся ревизии в других сферах деятельности. Мы также обозначим некоторые «подводные камни» ревизий, которые легко не заметить.

Введение


Ревизии кода известны достаточно давно. Они являются выжными спутниками движения свободного программного обеспечения с самого начала в форме, как минимум, права проверки патча перед его принятием.

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

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

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

Эта статья не претендует на суровую академичность. Это лишь набор тезисов, призванных стимулировать дальнейшие исследования и эксперименты.

Что такое ревизия кода?


Говоря кратко, ревизия кода это то, что происходит, когда кто-то просматривает ваш код и делает относительно него замечания.

Выделим некоторые виды ревизий.

В первую очередь стоит упомянуть о предварительных обзорах, т.е. ревизиях патчей перед их внесением в основную ветку кода проекта. Классический пример таких ревизий в сообществе свободного ПО – размещение патча новым разработчиком в списке рассылки. В некоторых проектах предварительные обзоры применяются даже к коду опытных разработчиков (например, Twisted, Bazaar, Linux).

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

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

Наконец, ревизии могут быть незапланированными. Это обычно случается поздней ночью, когда программист возится с кодом в каком-нибудь дальнем углу проекта, возможно отлавливая там ошибки. Такие ревизии обычно сопровождаются нецензурными восклицаниями и гневными комментариями. Зачастую они совершенно бесполезны для автора кода, поскольку ему вряд ли повезёт их услышать.

Для чего нужны ревизии кода?


Некоторым ревизии кода кажутся чем-то вроде написания документации, проектирования посредством тестирования или употребления в пищу отрубей: хорошо, если удаётся заниматься этими скучными вещами реже, чем требуется.

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

Усиление согласованности

Ревизии незаменимы, когда необходимо, чтобы весь исходный код выглядел единообразно. Это касается соглашений об именовании, проверке орфографии в комментариях и согласованности при использовании внутренних API.

Гарантии качества

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

Повышение ясности

Если участок кода просмотрен кем-то кроме его автора, это значит, что в мире существует как минимум два человека, которые способны его понять.

Опять же, ревизор интеллектуально более удалён от кода и потому может увидеть заложенные автором скрытые допущения и спорные решения.

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

Обучение рецензентов

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

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

Выравнивание приоритетов

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

Обучение программистов

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

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

Выводы

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

Решения


При внедрении процедуры ревизии кода возникают некоторые вопросы, требующие ответов. Ниже мы рассмотрим такие вопросы и увидим, как на них ответили в проектах Bazaar, Launchpad и Twisted.

Кому доверить рецензирование?

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

В проекте Bazaar патчи должны быть одобрены двумя основными разработчиками (т.е. теми, кому явно дано право размещения кода в репозитории). Этот вариант гарантирует, что все патчи будут просмотрены кем-то обладающим достаточным опытом, а также обеспечивает необходимый уровень общения между разработчиками.

В Launchpad в команде разработчиков выделена специальная группа рецензирования. Группа нацелена на то, чтобы включить в свой состав как можно большее число разработчиков, однако для получения полноценного права рецензирования необходимо пройти процедуру обучения с наставником. Эта процедура гарантирует, что в случае сомнения, новоиспечённый рецензент сможет обратиться к своему наставнику, и что приоритеты проекта (ясности кода и быстрота обзоров) будут поддерживаться всеми проверяющими.

В Twisted ревизию может выполнить кто угодно, кроме тех людей, кто принимал участие в создании патча. Эта хитроумная идея стимулирует групповое мышление автора и рецензентов. С другой стороны, это приводит к разброду в отзывах рецензентов, а сами обзоры кода, являясь «общим делом» быстро могут превратиться в «ничьё дело».

Как организовать форум?

В Twisted ревизии основываются на сообщениях об ошибках (bug tickets). В Bazaar проводят ревизии в общем списке рассылки. В Launchpad используют для этого отдельный список рассылки.

Подход Twisted – типичный пример UQDS (the Ultimate Quality Development System, универсальной системы обеспечения качества). Инспекции выполняются на основании заявок, а веб-страница с заявкой становится средоточием всей информации об исправлении ошибки или добавлении новой функции. Помимо централизованного хранения, этот способ позволяет отстранить от участия в обсуждении тех, кто не участвует в решении проблемы. Однако здесь есть и большой недостаток: отзывы рецензентов редко читает кто-нибудь кроме автора патча, что приводит к применению различных стандартов кодирования в каждом конкретном случае. Когда же в обзоре возникает действительно важный вопрос, привлечь у нему всеобщее внимание бывает очень сложно. К тому ж, неудачная система рассылок в Trac усложняет слежение за ходом дискуссии.

В Bazaar рецензии отсылаются прямо в общий список рассылки и учитываются в системе ведения патчей Bundle Buggy. Хотя обзоры и патчи напрямую рассылаются разработчикам по электронной почте, их обсуждения обычно бывают гораздо более открытыми, чем в Twisted. Однако, большое количество патчей затрудняет чтение списка рассылки, а обсуждения обзоров могут быть длинными и витиеватыми.

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

В реальном времени или в off-line?

Проведение «асинхронных» ревизий сильно отличается от ревизий в реальном времени.

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

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

Кто рассудит спорящих?

Случается, что автор и рецензент имеют прямо противоположные взгляды на код и не могут придти к соглашению. Например, автор считает что следующий код
 [item] = singleton_list
является очевидным вариантом получения единственного значения из списка, содержащего единственный элемент. Рецензент посчитал это неочевидным и предложил следующее:
 assert len(singleton_list) == 1, \
     "List should have only one value: %r" % (singleton_list,)
 item = singleton_list[0]


Несмотря на обоюдные доводы, стороны так и не пришли к согласию. Что делать? Кто разрешит противоречие?

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

Если «последнее слово» предоставить автору, то процесс программирования будет идти быстрее за счёт потерь в единообразии и, возможно, качестве. Если же последнее слово за рецензентом, то дополнения будут внедряться медленнее (рецензент не так одержим желанием внести патч, как его автор), но зато будут досконально изучены.

Что такое плохо


Ad hominem (переход на личности – прим. перев.)

Здесь практически нечего добавить: инспектируйте программу, а не программиста. Замечания о его личности лишь усложняют восприятие им критики.
Совет: если вы пишите негативные комментарии, пишите «патч» или «код» вместо «ты». Например, вместо «У тебя глюк в get_message» пишите «После применения этого патча в get_message появился глюк».

Туманные результаты ревизии

Если всё, что вы можете сказать, это «В этом патче появился глюк в get_message», то вы плохой рецензент. Цель ревизии – улучшить код, а не загадывать автору загадки.

Автор должен иметь возможность однозначно выделить и понять каждую отдельную мысль в вашем замечании, а также всё замечание целиком. Обзоры с нечёткими результатами превращаются в бесконечные дискуссии о коде, которые скорее служат тому, чтобы доставить удовольствие рецензенту, чем повышению качества кода.

Не путайте личные предпочтения с объективными достоинствами

Это проблема обозревателей в любой области: кинокритика, литературного редактора, научного рецензента. Не обязательно на самом деле хорошо то, что нравится мне. И уж тем более не обязательно на самом деле плохо то, что не нравится мне. Один из аспектов становления выдающегося программиста это моделирование собственных предпочтений в соответствии с реальными, объективными достоинствами.

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

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

См. также «Чёткий ответ – хороший ответ».

«Пока вы ещё здесь…»

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

Лучший совет в такой ситуации: дорожить поэтапным улучшением. См. «Лучшее враг хорошего».

«Флибустьерство»

«Флибустьерство» (filibustering) это американский политический термин, обозначающий затягивание дебатов по законопроекту с целью помешать его принятию. Иногда это делается и в проектах свободного ПО с целью саботировать принятие определённого патча.

«Флибустьерство» также может возникать непреднамеренно. Человек, проводящий ревизию, не принимает патч по причине отсутствия блочных тестов, автор спрашивает, как ему написать блочные тесты для этого кода, а рецензент так и не удосуживается ответить. А ведь омимо этих неприятностей, у автора есть ещё и свои дела, мешающие ему продолжать присылать новые патчи.

См. «Туманные итоги обзора» и «Быстрый ответ – хороший ответ».

Что такое хорошо


Чёткий ответ – хороший ответ

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

Быстрый ответ – хороший ответ

Как только автор представил свои патчи на ревизию, он больше не может над ними работать. Он должен дождаться ответа ревизора, прежде чем продолжить. Пишите свои замечания в обзор как можно скорее, чтобы внимание автора не успело рассеяться.

Лучшее враг хорошего

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

Будьте благодарны

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

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

Задавайте вопросы

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

Заключение


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

Что почитать по теме

(Также рекомендую ознакомиться с главой 21 «Совершенного кода» Стива Макконнелла – прим. перев.).

P.S. Перенёс в блог «Ревизия кода».
Михаил Клименко @michaelkl
карма
26,0
рейтинг 0,1
Реклама помогает поддерживать и развивать наши сервисы

Подробнее
Реклама

Самое читаемое Разработка

Комментарии (29)

  • НЛО прилетело и опубликовало эту надпись здесь
    • +1
      Спасибо за отзыв.

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

      Однако, статья всё-таки очень беглая и поверхностная.
      • +1
        Но тем не менее статья раскрывает важную истину. Кому нужна конкретика — теперь знает в каком ключе копать.
  • +10
    «Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live» (С)
    • +5
      Я приведу перевод:

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

      Стив Макконнелл «Совершенный код»
  • 0
    Недавно, ставя процесс разработки в компании всего четырех человек, столкнулись с подобной проблемой. А именно: один человек пишет, второй полночи рецензирует и правит, с утра второй выдает претензии первому, а тот кричит о том, что если у него такой плохой код, то почему он работает, и если его постоянно переделывают — то зачем он вообще работает?) И было много всего, что тут описано, причем проблема решилась, по крайней мере приостановилась, за счет как раз подхода XP — их посадили в пару.
    Считаю, впрочем, что рецензирование важно для всех участников процесса, по крайней мере нам, как мне кажется, оно дало очень многое, хотя и не без описанных проблем. Но пользы точно больше)
  • 0
    Спасибо за ссылку на выступление Гвидо
  • 0
    Иногда просто с ума сходишь, когда на проекте меняется программист.
    На проект потрачено 400 часов, и тут он тебе заявляет, что по-хорошему надо всё переписать: «Выделите только мне на это несколько часов».
    В итоге ты не соглашаешься и оказывается, что через неделю он все-таки умудрился всё это переписать :)
  • 0
    Спасибо за перевод, было интересно почитать.

    Сейчас подумываю о том, что как только появится больше опыта, будет полезно присоединиться к какому-нибудь небольшому open source проекту, но вот нигде не встречал статей или руководств «как начать»…
    • +1
      а что тут сложного? находишь проект который тебе нравится смотришь в местный багтрекер и чтонить простое делаешь. О том как извлекать код из репозитория и делать патчи мануалов море.
      Тут главное начать и не боятся сложностей. Всегда есть куча мелких багов до которых руки не доходят. А для неопытных они самое то.
    • 0
      К нам! К нам на зеленые ноутбуки и маемовские таблетки! osll.spb.ru
  • +1
    А слово «ревизия» вы сами придумали? Просто обычно так переводится слово revision, которое есть единичное изменение в системе контроля версий. Даже не сразу понял что вы о code review речь ведёте.
    • 0
      В глоссарии для subversion это слово переводится как «правка»
    • +1
      А как же гоголевский Ревизор? =)
      Там понятие ревизии было
      • 0
        Я же не спорю с тем, что такое слово есть. Но использование его в качестве перевода code review мне режет слух и глаз. Возможно, только мне :)
    • +1
      О, хороший вопрос! :)
      Вообще, я долго думал, как перевести «code review». Было три варианта:
      * обзор кода;
      * инспекция кода;
      * ревизия кода.

      Тогда я открыл Википедию, ввёл туда «code review» и получил вот эту ссылку. Там как раз и написано «ревизия».
      • 0
        Кстати, мне бы инспекция кода была понятнее — в одном из тулов на английском этот процесс назывался именно code inspection.
        • +1
          Кому-то понятнее один вариант, кому-то – другой, кто как привык.

          Отсутствие общепринятого перевода некоторых IT-терминов действительно создаёт проблемы, приходится смотреть по контексту. Те же «design patterns» кто-то называет шаблонами, а кто-то паттернами.

          А в нашем случае даже в англоязычных терминах нет согласья: в статье «review», у вас — «inspection».

          Что поделать – «трудности перевода» ©. :)
          • 0
            Я тоже пока не прочитал несколько абзацев не мог понять о чём идёт речь. За ревизией уже закрепилось значение как «версия» кода. потому лучше всё таки было использовать другие варианты. Та же инспекция кода.
  • 0
    Классно! Спасибо за перевод, нашел у кого-то в блоге ссылку на статью, было лень читать на английском. :)
  • 0
    Отличная статья. Вот бы еще и инструмент хороший для ревью кода, а за одно и для подготовки патчей с отложенным комитом для ревью под SVN или CVS. Я делаю ревью кода разработчиков при чекауте. В этом случае код уже в репозитории и процедура его изменения (пинать автора или писать ему письмо) очень трудоемкое занятие. Я видел пару платных инструментов типа гугловского мандриана (видел демо около года назад и оценил общую концепцию) но они заточены под систему контроля версий которая умеет делать отложеные комиты.
    • 0
      Посмотрите на Git. Попробуйте схему, когда патчи мёрждат рецензенты.
      • 0
        Спасибо, гит я использовал. Не хватает пока ему нормальной визуальной поддержки, а заливаться из текстовой консоли напрягает. (раньше приходилось для него cygwin ставить)
        • 0
          Пробовали tortoisegit?

          Сам не кушал, но ходят слухи, что программа уже вполне на уровне.

          Да и, признаться, консоль не так плоха. Любимые команды можно заальясить, стандартные операции автоматизировать скриптами. Git настолько хорош, что его вроде как уже даже умудрились на C# портировать.
    • +1
      smartbear.com/
      Не без недостатков, но хорошая штука. Хотя и тоже платная. Впрочем, кряки находятся без проблем :)

      позволяет ревьюить локальные изменения до чек-ина для систем, которые неумеют отложенные коммиты.

      Мы в итоге купили, поюзав месяца с два.
      • 0
        да, smartbear видел, хорошо сделали, вот именно такой системы и не хватает для полного счастья. Кстати сейчас у них акция лицензия на 5 юзеров за 5 баксов. Сейчас куплю.
  • 0
    В MediaWiki используется специальная система. Право ставить статус «ok» (или resolved) имеют права два главных разработчика. Право ставить fixme и другие статусы имеют почти все основные разработчики.

    Процесс обновления движка на Википедии выглядит так. Когда у основных разработчиков появляется время, то они просматривают все изменения и ставят ok/fixme/reverted. Потом, когда все fixme исправляются, движок обновляется к новой версии (при этом часто возникает необходимость обновить структуру базы данных, что может быть весьма долго и трудно) и, после проверки в живых условиях на специальном сайте, запускаются глобально.
  • 0
    говорящая опечатка — «повешению качества кода»…
    • 0
      Спасибо, исправил.

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