Хочу представить свой перевод статьи «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, например, по электронной почте, у рецензента есть время чтобы дипломатично сформулировать свои замечания, а автор может отвечать на них в любом удобном ему порядке.
Ревизии «в реальном времени» повторяют все достоинства и недостатки обычного разговора: легко обнаруживаются недопонимания, а ответа не приходится долго ждать. Однако могут быстро накаляться страсти, что мешает отделять факты от эмоций. К тому же сложнее определить продолжительность таких ревизий. Получив письмо, вы всегда можете увидеть его размер в почтовом клиенте, а общение в реальном времени может продолжаться неопределённо долго.
Случается, что автор и рецензент имеют прямо противоположные взгляды на код и не могут придти к соглашению. Например, автор считает что следующий код
является очевидным вариантом получения единственного значения из списка, содержащего единственный элемент. Рецензент посчитал это неочевидным и предложил следующее:
Несмотря на обоюдные доводы, стороны так и не пришли к согласию. Что делать? Кто разрешит противоречие?
В Bazaar принято, что право сказать последнее слово имеет автор кода, в Twisted наоборот, последнее слово принадлежит рецензенту, в Launchpad же решающим голосом в таких вопросах обладает специально назначенный главный рецензент.
Если «последнее слово» предоставить автору, то процесс программирования будет идти быстрее за счёт потерь в единообразии и, возможно, качестве. Если же последнее слово за рецензентом, то дополнения будут внедряться медленнее (рецензент не так одержим желанием внести патч, как его автор), но зато будут досконально изучены.
Здесь практически нечего добавить: инспектируйте программу, а не программиста. Замечания о его личности лишь усложняют восприятие им критики.
Совет: если вы пишите негативные комментарии, пишите «патч» или «код» вместо «ты». Например, вместо «У тебя глюк в
Если всё, что вы можете сказать, это «В этом патче появился глюк в
Автор должен иметь возможность однозначно выделить и понять каждую отдельную мысль в вашем замечании, а также всё замечание целиком. Обзоры с нечёткими результатами превращаются в бесконечные дискуссии о коде, которые скорее служат тому, чтобы доставить удовольствие рецензенту, чем повышению качества кода.
Это проблема обозревателей в любой области: кинокритика, литературного редактора, научного рецензента. Не обязательно на самом деле хорошо то, что нравится мне. И уж тем более не обязательно на самом деле плохо то, что не нравится мне. Один из аспектов становления выдающегося программиста это моделирование собственных предпочтений в соответствии с реальными, объективными достоинствами.
Если вы рецензируете патч, который прекрасно устраняет какую-либо проблему, используя парадигму императивного программирования, то не стоит критиковать его лишь за то, что он не следует функциональной парадигме. Так вы дискредитируете идею ревизий, превращаю её в игру по угадыванию предпочтений рецензента, а она должна способствовать создания действительно хорошего код.
Формулирование комментариев в виде вопросов поможет проверяющему избежать такой оплошности, например: «Не думали ли вы использовать функциональный стиль?», «Почему вы не используете регулярные выражения для решения этой задачи?» и т.д.
См. также «Чёткий ответ – хороший ответ».
При обзоре кода всегда есть соблазн попросить автора исправить заодно и какие-нибудь другие проблемы. Такой подход может быстро перерасти в полномасштабную операцию по наведению глянца на большом участке кода, хотя изначально автор и проверяющий должны были утвердить лишь небольшое улучшение.
Лучший совет в такой ситуации: дорожить поэтапным улучшением. См. «Лучшее враг хорошего».
«Флибустьерство» (filibustering) это американский политический термин, обозначающий затягивание дебатов по законопроекту с целью помешать его принятию. Иногда это делается и в проектах свободного ПО с целью саботировать принятие определённого патча.
«Флибустьерство» также может возникать непреднамеренно. Человек, проводящий ревизию, не принимает патч по причине отсутствия блочных тестов, автор спрашивает, как ему написать блочные тесты для этого кода, а рецензент так и не удосуживается ответить. А ведь омимо этих неприятностей, у автора есть ещё и свои дела, мешающие ему продолжать присылать новые патчи.
См. «Туманные итоги обзора» и «Быстрый ответ – хороший ответ».
Указывайте конкретные строки кода, которые можно улучшить. Опишите, что с в них не так. Предложите способ сделать лучше. Помогите автору определить пределы совершенствования.
Как только автор представил свои патчи на ревизию, он больше не может над ними работать. Он должен дождаться ответа ревизора, прежде чем продолжить. Пишите свои замечания в обзор как можно скорее, чтобы внимание автора не успело рассеяться.
Нельзя сделать идеальный патч, который бы разом решил все ваши проблемы. Не гонитесь за идеалом, работайте над поэтапным улучшением.
Кто-то попытался улучшить ваш продукт. Они потратили свои силы и фантазию на то, чтобы помочь вам, а теперь представили свой труд на суд общественности: так поблагодарите их.
В проекте Divmod практикуют принцип включения в каждый обзор кода хотя бы одной похвалы. Всегда можно найти, за что похвалить автора, пусть даже лишь за то, что он взялся поработать над этой частью кода.
Рецензент должен задавать вопросы. Нельзя ошибиться, задавая вопросы. В худшем случае автор даст тривиальный ответ. В лучшем случае оба, и автор, и проверяющий, откроют для себя что-то новое.
Ревизии кода дают человеку уникальную возможность расти как программисту, при этом улучшая продукт, который он создаёт. Каждый проект может выбирать из многих вариантов проведения ревизий и предоставляемых ими возможностей. Тщательное обдумывание технологий и процессов взаимодействия людей при проведении обзоров позволит проектам с открытым кодом избежать сложностей, которые отпугивают новичков и утомляют давнишних участников, и сосредоточиться на разработке наилучшего программного обеспечения.
P.S. Перенёс в блог «Ревизия кода».
Я ненавижу тебя: твой код – хлам!
Взаимоотношения участников ревизий кода
Джонатан Лэндж (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 практикуют принцип включения в каждый обзор кода хотя бы одной похвалы. Всегда можно найти, за что похвалить автора, пусть даже лишь за то, что он взялся поработать над этой частью кода.
Задавайте вопросы
Рецензент должен задавать вопросы. Нельзя ошибиться, задавая вопросы. В худшем случае автор даст тривиальный ответ. В лучшем случае оба, и автор, и проверяющий, откроют для себя что-то новое.
Заключение
Ревизии кода дают человеку уникальную возможность расти как программисту, при этом улучшая продукт, который он создаёт. Каждый проект может выбирать из многих вариантов проведения ревизий и предоставляемых ими возможностей. Тщательное обдумывание технологий и процессов взаимодействия людей при проведении обзоров позволит проектам с открытым кодом избежать сложностей, которые отпугивают новичков и утомляют давнишних участников, и сосредоточиться на разработке наилучшего программного обеспечения.
Что почитать по теме
- Guido Van Rossum on Mondrian, собственный инструмент обзора кода Google
- The Ultimate Quality Development System
- The Bazaar HACKING Guide
- Beyond copy-editing: the editor-writer relationship
P.S. Перенёс в блог «Ревизия кода».