Считается ли плохой практикой включать номер ошибки в имя метода для временного решения проблемы?

27

Мой коллега, который является старшим, блокирует меня при проверке кода, потому что он хочет, чтобы я назвал метод PerformSqlClient216147Workaround, потому что это обходной путь для некоторого дефекта ###. Теперь, мое предложение имени метода является чем-то вроде PerformRightExpressionCast, которое имеет тенденцию описывать, что метод фактически делает. Его аргументы идут по линии: «Ну, этот метод используется только как обходной путь для этого случая, и нигде больше».

Будет ли включение номера ошибки внутри имени метода для временного обходного пути считаться плохой практикой?

Боян
источник
Просто пояснение: дефект ### находится во внешнем компоненте с именем SqlClient, он был подан в 2008 году, скорее всего, он не будет исправлен в ближайшее время, и он не в наших силах, поэтому этот метод является частью разработки для " Обходной путь ", что проблема ...
Боян
2
Это все еще читалось как напыщенная речь, поэтому я перефокусировал и переименовал вопрос в суть того, что вы спрашиваете. Я чувствую, что это справедливый вопрос сейчас. Такие вопросы, как "Мой начальник сделал X, он так неправ ... ПРАВИЛЬНЫЕ ПАРНИ?" как правило, закрыты как неконструктивные.
maple_shaft
41
Предположим, временный обходной путь станет постоянным. Они всегда делают.
user16764
2
@maple_shaft - отличное сохранение-редактирование вопроса.
2
Ошибки # предназначены для комментариев и примечаний к коммиту управления версиями, а не имен методов Твоему коллеге следует дать пощечину.
Эрик Реппен

Ответы:

58

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

/**
 * Cast given right-hand SQL expression.
 *
 * Note: This is a workaround for an SQL client defect (#216147).
 */
public void CastRightExpression(SqlExpression rightExpression)
{
    ...
}

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

Бернард
источник
5
Было бы неплохо иметь прямую http-ссылку на ошибку в комментарии к документации. Вы также можете определить свои собственные аннотации@Workaround(216147)
Sulthan
2
или @warning This is a temporary hack to...илиTODO: fix for ...
BЈовић
1
@Sulthan - Конечно ... Позволяет ссылку на базу данных дефектов, которая может не существовать в течение нескольких лет. Опишите дефект, укажите номер дефекта (и дату), запишите его обходной путь, но ссылки на внутренние инструменты, которые могут измениться, кажутся плохой идеей.
Ramhound
4
@Ramhound Вы должны рассматривать свою базу данных дефектов и историю изменений как минимум такую ​​же ценную, как исходный код. Они рассказывают вам полную историю того, почему что-то было сделано, и как это должно быть так. Следующий человек должен будет знать.
Тим Уиллискрофт
1
Код (в данном случае имя метода) должен самодокументировать, что он делает. Комментарии должны объяснять, почему код существует или структурирован определенным образом.
Аарон Курцхалс
48

Нет ничего более постоянного, чем временное исправление, которое работает.

Его предложение выглядит хорошо через 10 лет? Обычно принято комментировать каждое изменение с исправленным дефектом. Совсем недавно (как и за последние 3 десятилетия) этот стиль комментирования получил широкое признание как уменьшение возможности сопровождения кода - и это просто комментарии, а не имена методов.

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

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

mattnz
источник
40
+1Nothing is more permanent than a temporary fix that works.
Reactgular
2
"широко принят" [цитата нужна]
3
@ Грэхем: Это достаточно хорошо, или это нужно для рецензирования, опубликованной в авторитетном журнале ... stackoverflow.com/questions/123936/…
mattnz
14

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

Если это не так, вы всегда можете сказать, что 216147в коде нет смысла, так как код не связан с системой отслеживания ошибок (скорее это система отслеживания ошибок, которая связана с системой контроля версий). Исходный код не является хорошим местом для ссылок на тикеты и версии ошибок, и если вам действительно нужно разместить эти ссылки там, сделайте это в комментариях.

Обратите внимание, что даже в комментариях только номер ошибки не очень ценен. Представьте себе следующий комментарий:

public IEnumerable<Report> FindReportsByDateOnly(DateTime date)
{
    // The following method replaces FindReportByDate, because of the bug 8247 in the
    // reporting system.
    var dateOnly = new DateTime(date.Year, date.Month, date.Day);
    return this.FindReportByDate(dateOnly);
}

private IEnumerable<Report> FindReportsByDate(DateTime date)
{
    Contract.Requires(date.Hour == 0);
    Contract.Requires(date.Minute == 0);
    Contract.Requires(date.Second == 0);

    // TODO: Do the actual work.
}

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

Вывод: вы понятия не имеете, о чем эта ошибка.

Один и тот же код мог быть написан немного по-другому:

public IEnumerable<Report> FindReportsByDateOnly(DateTime date)
{
    // The reporting system we actually use is buggy when it comes to searching for a report
    // when the DateTime contains not only a date, but also a time.
    // For example, if looking for reports from `new DateTime(2011, 6, 9)` (June 9th, 2011)
    // gives three reports, searching for reports from `new DateTime(2011, 6, 9, 8, 32, 0)`
    // (June 9th, 2011, 8:32 AM) would always return an empty set (instead of isolating the
    // date part, or at least be kind and throw an exception).
    // See also: http://example.com/support/reporting-software/bug/8247
    var dateOnly = new DateTime(date.Year, date.Month, date.Day);
    return this.FindReportsByDate(dateOnly);
}

private IEnumerable<Report> FindReportsByDate(DateTime date)
{
    Contract.Requires(date.Hour == 0);
    Contract.Requires(date.Minute == 0);
    Contract.Requires(date.Second == 0);

    // TODO: Do the actual work.
}

Теперь вы получите четкое представление о проблеме. Даже если кажется, что гипертекстовая ссылка в конце комментария не работает пять лет назад, это не имеет значения, поскольку вы все равно можете понять, почему ее FindReportsByDateзаменили FindReportsByDateOnly.

Арсений Мурзенко
источник
Хорошо, мы кое-что получаем: как вы думаете, почему исходный код не подходит для номеров ошибок?
Боян
1
Потому что системы отслеживания ошибок и контроля версий - лучшее место для этого. Это не совсем то же самое, но похоже на: stackoverflow.com/q/123936/240613
Арсений Мурзенко
Хорошо, имеет смысл в целом. Но если вы имеете дело с обходным решением, как в случае отклонения от основного дизайна, я думаю, что можно оставить комментарий, объясняющий, что было сделано (и, возможно, номер дефекта в комментариях), чтобы кто-то, кто читает код, мог понять почему что-то было сделано определенным образом.
Боян
2
Я думал, что только маркетологи могут утверждать логику добавления чего-то нового, что устарело.
Mattnz
1
Если почему код делает то, что он делает, чтобы обойти ошибку, не понятно из прочтения, и вам нужно длинное объяснение того, почему обходной путь делает то, что он делает, включая ссылку на то, где внешняя документация может быть найдена в комментарии, является разумным IMO , Да, вы можете использовать инструмент обвинения в управлении исходным кодом, чтобы выяснить, к какому изменению был добавлен обходной путь, и получить объяснение там, но с большой базой кода, и особенно после того, как рефакторинг в другом месте закончится, возившись с обходным решением, выполнение которого может занять много времени. ,
Дэн Нили,
5

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

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

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

Reactgular
источник
2
Я согласен, так как кажется, что PerformSqlClient216147Workaround описывает как именно метод, так и причину его существования. Я бы пометил его атрибутом (C #), специфичным для таких вещей для вашего магазина, и покончим с этим. Числа имеют свое место в именах ... хотя, как и выше, надеюсь, это не только методология, которую ваш магазин использует для классификации таких вещей. Буря в стакане ИМХО. Кстати, это реальный код ошибки ?? Если это так, вы, старший сотрудник, вероятно, имеете шанс открыть этот пост, что может быть или не быть проблемой ... для вас. ;)
RISM
3

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

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


источник
3
это следует объяснять в комментариях к методу, а не к его названию.
Арсений Мурзенко
2
Я согласен с вашим ответом в целом, но я также согласен с MainMa: мета-информация о методе должна быть в комментариях, а не в названии.
Роберт Харви
3

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

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

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

В любом случае, у вас с ним есть дела поважнее.

Стивен С
источник
Дело принято. Но я бы не стал недооценивать силу эго :)
Боян
1

Будет ли включение номера ошибки внутри имени метода для временного обходного пути считаться плохой практикой?

Да.

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

Если часть общедоступного API этого класса в основном говорит «выполнить операцию Y, описанную в документе / месте X», то способность других людей понимать API будет зависеть от:

  1. они знают, что искать во внешней документации

  2. они знают, где искать внешнюю документацию

  3. они тратят время и усилия и на самом деле глядя.

С другой стороны, имя вашего метода даже не упоминает, где "местоположение X" для этого дефекта.

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

По крайней мере, если вы знаете, что дефект всегда будет доступен в одном и том же месте, и он не изменится (например, номер дефекта Microsoft, который был по тому же URL-адресу в течение последних 15 лет), вы должны предоставить ссылку на проблема в документации API.

Тем не менее, могут существовать обходные пути для других дефектов, которые влияют не только на API одного класса. Что ты будешь делать тогда?

Наличие API с одним и тем же номером дефекта в нескольких классах (на data = controller.get227726FormattedData(); view.set227726FormattedData(data);самом деле мало о чем говорит, а просто делает код более неясным.

Вы можете решить, что все другие дефекты устранены с помощью имен, описывающих операцию ( data = controller.getEscapedAndFormattedData(); view.setEscapedAndFormattedData(data);), за исключением случая вашего дефекта 216147 (который нарушает принцип проектирования «наименьший сюрприз» - или, если вы хотите выразиться так, он увеличивает измерение "WTFs / LOC" вашего кода).

TL; DR: плохая практика! Иди в свою комнату!

utnapistim
источник
0

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

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

Джеймс Андерсон
источник
0

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

Если часть взаимодействия с SqlClient требует от вас правильного приведения выражения, тогда ваш код должен читать «executeRightExpressionCast ()». Вы всегда можете прокомментировать код, чтобы объяснить, почему.

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

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

Последствия:

  1. Как правило, меньше кода в результате.
  2. Простой код
  3. Меньше ссылок на ошибки в исходном коде
  4. Меньший риск регрессии в будущем (после полной проверки изменения кода)
  5. Проще анализировать, потому что разработчики не должны обременять себя историей обучения, которая больше не актуальна.

Исходный код не должен сообщать мне, как он попал в его текущее состояние. Контроль версий может рассказать мне историю. Мне нужен исходный код, чтобы просто быть в состоянии, необходимом для работы. Тем не менее, случайный комментарий "// ошибка 12345" не является плохой идеей, но его злоупотребляют.

Поэтому, когда вы решаете, называть ваш метод PerformSqlClient216147Workaround или нет, задайте себе следующие вопросы:

  1. Если бы я знал об ошибке 216147 до написания кода, как бы я ее обработал?
  2. Какой обходной путь? Если бы кто-то, кто никогда не видел этот код раньше, взглянул бы на него, смогут ли они следовать ему? Нужна ли проверка системы отслеживания ошибок, чтобы знать, как работает этот код?
  3. Насколько временен этот код? По моему опыту, временные решения обычно становятся постоянными, как указывает @mattnz.
Brandon
источник