Является ли обзор кода, который использует только комментарии кода хорошей идеей?

16

Предпосылками

  • Команда использует DVCS
  • IDE поддерживает анализ комментариев (например, TODO и т. Д.)
  • Такие инструменты, как CodeCollaborator, дороги для бюджета
  • Инструменты типа gerrit слишком сложны для установки или не пригодны для использования

Workflow

  • Автор публикует где-то в центральной ветке репо
  • Рецензент получить его и начать обзор
  • В случае возникновения какого-либо вопроса / рецензента, создайте комментарий со специальной меткой, например, «REV». Такой ярлык НЕ ДОЛЖЕН быть в рабочем коде - только на этапе проверки:

    $somevar = 123;
    // REV Why do echo this here?
    echo $somevar;
    
  • Когда рецензент заканчивает публиковать комментарии - он просто коммит с глупым сообщением "комментарии" и отталкивает назад

  • Автор возвращает ответвление функции и отвечает на комментарии аналогичным образом или улучшает код и возвращает его обратно.
  • Когда комментарии «REV» исчезли, мы можем думать, что обзор успешно завершен.
  • Автор в интерактивном режиме перебазирует ветку функции, раздавливает ее, чтобы удалить эти коммиты «комментария», и теперь готов объединить функцию для разработки или выполнения любого действия, которое обычно может быть после успешного внутреннего просмотра

Поддержка IDE

Я знаю, что пользовательские теги комментариев возможны в eclipse & netbeans. Конечно, это также должно быть в семье BlablaStorm.

Пример настраиваемой отфильтрованной задачи из комментариев в Eclipse

Вопросов

  1. Как вы думаете, эта методология жизнеспособна?
  2. Вы знаете что-то подобное?
  3. Что в этом можно улучшить?
gaRex
источник
Хороший вопрос, но он не имеет ничего общего с салфетками - не очень хороший заголовок.
MarkJ
@MarkJ как назвать это тогда? Я имею в виду что-то ремесленное, дачное, домашнее. По-русски у нас есть фраза "на коленке". Что-то не стабильное, не идеальное, не твердое, как, но это работает.
gaRex
2
@MarkJ, gaRex: как насчет этого нового названия? Не стесняйтесь отменить, если вы считаете, что это не подходит для этого вопроса.
Арсений Мурзенко
@MainMa, все в порядке
gaRex
1
Atlassian Crucible является по сути бесплатным для 10 разработчиков. Это также лучший инструмент для проверки кода, который я использовал. Подход с использованием комментариев является жизнеспособным, но может усложнить отслеживание состояния.
Эндрю Т Финнелл

Ответы:

14

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

Есть еще несколько недостатков:

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

  • Я не верю, что рецензент имеет достаточно отзывов от рецензента, чтобы знать, как на самом деле были применены рецензированные пункты .

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

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

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

    В более формальном процессе проверки Алиса сразу увидела, что она сделала замечание, и увидела разницу в соответствующем коде, чтобы заметить, что Эрик неправильно понял синтаксис, о котором она ему рассказала.

  • Люди все еще люди. Я почти уверен, что некоторые из этих комментариев окажутся в рабочем коде, а некоторые останутся мусором, будучи полностью устаревшими .

    Конечно, та же проблема существует с любым другим комментарием; например, в производстве есть много комментариев TODO (включая самый полезный: «TODO: прокомментируйте код ниже.») и множество комментариев, которые не были обновлены, когда был соответствующий код.

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

  • Это не заменяет очную проверку (но та же проблема относится и к любой другой, более формальной проверке, которая не проводится лицом к лицу).

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

  • Вы также можете столкнуться с незначительными проблемами с синтаксисом (который также существует для комментариев TODO). Например, что если длинный комментарий "// BLA" появляется в несколько строк, и кто-то решает написать его следующим образом:

    // BLA This is a very long comment which is way beyond 80 characters, so it actually
    // fills more then one line. Since the next lines start with slashes, but not "BLA"
    // keyword, the IDE may not be able to show those lines, and display only the first one.
    
  • И, наконец, небольшая и очень личная заметка: не выбирайте «BLA» в качестве ключевого слова. Звучит ужасно. ;)

Арсений Мурзенко
источник
«знать, как на самом деле применялись проверенные пункты» Да :) Только честность рецензента. Здесь у нас больше политики, чем инструмента.
gaRex
1
"люди есть люди" Да :( У нас уже есть миллионы таких TODO. Может быть, просто отрицать, что такая функция есть в IDE?
gaRex
«Загрязнить журнал контроля версий». Для этого вся работа находится в отдельной ветке функций, которая позже будет раздавлена ​​и объединена в разработку. Может быть, это можно сделать простым подключением в DVCS. Но, как я вижу, вся сложность переходит от инструментов проверки кода к IDE и DVCS.
gaRex
"незначительные проблемы с синтаксисом" Может быть, это не проблема? Эти REV являются всего лишь некоторыми маркерами, чтобы быстро перейти к нему с панели.
gaRex
1
@gaRex: тогда члены вашей команды должны использовать электронную почту, чтобы договориться о дате свидания лицом к лицу ;-)
Док Браун
4

Я бы дополнил комментарии в коде сопроводительным документом. Это будет обобщать результаты и жить после того, как комментарии были удалены. Преимущества этого будут:

  • Компактность. Если человек обычно не может проверить, что переданный указатель не равен нулю (или есть какая-то другая распространенная ошибка новичка на используемом вами языке), рецензент может оставить десятки комментариев REV на этот счет, и в документе можно сказать « Я нашел 37 мест, где указатели не проверялись первыми, не перечисляя их все
  • место для похвалы. Такой комментарий REV this is a nice designкажется странным, но мои обзоры кода часто включают одобрение и исправления.
  • интерактивность. Ваш примерный комментарий: «Зачем ты это сделал?» и это очень типичный. Подход только для комментариев не поддерживает разговор. Человек может изменить свой код и удалить комментарий или удалить комментарий. Но "почему ты это сделал?" и "пожалуйста, измени это, это неправильно" - это не одно и то же.
  • ведение учета. Более поздний рецензент может проверить, действительно ли код был изменен, или комментарии были просто удалены. Они также могут проверить, что комментарии были «приняты к сведению», и разработчик больше не делает те же ошибки в последующем обзоре.

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

Кейт Грегори
источник
тогда нам нужен хороший инструмент для проверки кода. Наш нынешний описанный подход в основном политический, так как люди должны придумать какой-то этикет и следовать ему.
gaRex
«компактность» - я думаю, что это можно сделать одним комментарием вроде «// REV Dude, у нас есть 37 мест, где указатели не проверялись первыми, включая этот. Пожалуйста, RTFM»
gaRex
"место для похвалы" - также возможно, но после сквоша будет потеряно :( Я уже вижу одну проблему - мы потеряли историю обзора при фиксации
сквоша
«интерактивность» - автор может ответить в другом комментарии, ниже начального. Как в стиле википедии.
gaRex
«человек может ... удалить комментарий» - это проблема. +1
gaRex
2

Другие говорили об ограничениях этого подхода. Вы упомянули, что такие инструменты, как gerrit, было трудно установить - я предлагаю вам взглянуть на phabricator (http://www.phabricator.com). Это система проверки кода, которую Facebook использовал годами и недавно был открыт. Это не сложно установить, имеет отличные рабочие процессы и решает все проблемы, упомянутые в других комментариях.

Адам Хупп
источник
Вот это да! Мы должны попробовать это вместо нашей прекрасной красной мины.
gaRex
"Gerrit" Я установил его - это было не так сложно. Я просто не могу его использовать! А также имеет ужасный интерфейс и рабочий процесс.
gaRex
@gaRex Мы используем Reviewboard ( reviewboard.org ) параллельно с Redmine. Они служат разным целям, так что это нормально. И вы можете настроить RB для ссылки на проблемы Redmine ...
Йоханнес С.
@JohannesS. какие VCS вы используете? Также у вас есть готовые инструкции или вики об их интеграции? Приятно иметь такой.
gaRex
@gaRex Извините за поздний ответ. Мы используем SVN, но RB также поддерживает git (на самом деле, пользователи RB сами используют git). Предлагаю взглянуть на сайт РБ. Доступна онлайн-демонстрация (например, ознакомьтесь с demo.reviewboard.org/r/101 )
Йоханнес С.