Является ли это использование условных выражений анти-паттерном?

14

Я часто видел это в нашей унаследованной системе - функции, которые работают примерно так:

bool todo = false;
if(cond1)
{
  ... // lots of code here
  if(cond2)
    todo = true;
  ... // some other code here
}

if(todo)
{
  ...
}

Другими словами, функция состоит из двух частей. Первая часть выполняет какую-то обработку (потенциально содержащую циклы, побочные эффекты и т. Д.), И по ходу дела может установить флаг «todo». Вторая часть выполняется только в том случае, если был установлен флаг «todo».

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

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

Kricket
источник
2
Как вы рефакторинг это?
Тамас Селеи
13
Предполагая, что todo установлен в нескольких местах, в соответствии с несколькими нетривиальными неисключительными условиями, я едва ли могу думать о рефакторинге, который имеет хоть малейший смысл. Если нет рефакторинга, нет антипаттерна. За исключением именования переменной todo; должен быть назван более выразительным, как "doSecurityCheck".
user281377
3
@ammoQ: +1; если все сложно, то так оно и есть. Переменная flag может иметь гораздо больше смысла в некоторых обстоятельствах, поскольку она проясняет, что решение было принято, и вы можете найти его, чтобы найти, где это решение было принято.
Донал Феллоуз
1
@Donal Fellows: Если поиск причины необходим, я бы сделал переменную списком; до тех пор, пока он пуст, он «ложный»; Когда флаг установлен, код причины добавляется в список. Таким образом, вы можете закончить со списком, ["blacklisted-domain","suspicious-characters","too-long"]который показывает, что применено несколько причин.
user281377
2
Я не думаю, что это анти-шаблон, но это определенно запах
Binary Worrier

Ответы:

23

Я не знаю об анти-паттерне, но я бы извлек из этого три метода.

Первый будет выполнять некоторую работу и возвращать логическое значение.

Второй будет выполнять любую работу, выполняемую «другим кодом»

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

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

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

Что-то вроде этого:

public void originalMethod() {
    bool furtherProcessingRequired = lotsOfCode();
    someOtherCode();
    if (furtherProcessingRequired) {
        doFurtherProcessing();
    }
    return;
}

private boolean lotsOfCode() {
    if (cond1) {
        ... // lots of code here
        if(cond2) {
            return true;
        }
    }
    return false;
}

private void someOtherCode() {
    ... // some other code here
}

private void doFurtherProcessing() {
    // Do whatever is needed
}

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

Дело в том, что цель кода становится понятнее, и это хорошо ...

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

Билл Мичелл
источник
Разделение на 2 функции по-прежнему требует todoпеременной и, вероятно, будет сложнее для понимания.
Пабби
Да, я бы тоже это сделал, но мой вопрос был больше об использовании флага "todo".
Крикет
2
Если вы в конечном итоге if (check_if_needed ()) do_whatever ();, там нет очевидного флага. Я думаю, что это может разбить код слишком сильно и потенциально повредить читабельности, если код достаточно прост. В конце концов, детали того, что вы делаете, do_whateverмогут повлиять на то, как вы тестируете check_if_needed, поэтому полезно хранить весь код в одном и том же экране. Кроме того, это не гарантирует, что check_if_neededможно будет избежать использования флага - и если это произойдет, он, вероятно, будет использовать несколько returnоператоров для этого, возможно, расстроив строгих защитников с одним выходом.
Steve314
3
@ Pubby8 он сказал: «Извлеките 2 метода из этого» , в результате чего 3 метода. 2 метода, выполняющие фактическую обработку, и оригинальный метод, координирующий рабочий процесс. Это было бы намного чище.
MattDavey
Это опускается ... // some other code hereв случае раннего возврата
Caleth
6

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

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

bool registrationNeeded = installSoftware(...);
if (registrationNeeded) {
  registerUser(...)
}

Или вы можете даже полностью избавиться от локального флага, скрыв проверку флага во втором методе и используя такую ​​форму, как

calculateTaxRefund(isTaxRefundable(...), ...)

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

Петер Тёрёк
источник
4

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

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

bool requiresSorting = false;
if(cond1)
{
    ... // lots of code here
    if(cond2)
        requiresSorting = true;
    ... // some other code here
}

if(requiresSorting)
{
    ...
}

Тем не менее, предложение Билла также верно. Это еще более читабельно:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);
прецизионный самописец
источник
Почему бы просто не пойти еще дальше: if (BuildList (list)) SortList (list);
Фил Н Дебланк
2

Шаблон конечного автомата выглядит хорошо для меня. Здесь есть анти-паттерны "todo" (плохое имя) и "много кода".

ptyx
источник
Я уверен, что это только для иллюстрации.
Лорен Печтел
1
Согласовано. То, что я пытался донести, - это то, что хорошие шаблоны, утопленные в плохом коде, не должны быть обвинены в качестве кода.
Ptyx,
1

Это зависит на самом деле. Если код, охраняемый todo(я надеюсь, вы не используете это имя по-настоящему, поскольку оно совершенно не мнемонично!) Является концептуально очищающим кодом, то у вас есть анти-паттерн, и вам следует использовать что-то вроде RAII C ++ или C #. usingпостроить, чтобы обрабатывать вещи вместо этого.

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

Donal Fellows
источник
Это явно не очистка - это не всегда работает. Я сталкивался с подобными случаями раньше - при обработке чего-либо вы можете в итоге сделать недействительным какой-то предварительно вычисленный результат. Если расчет дорог, вы можете запустить его только при необходимости.
Лорен Печтел
1

Многие из ответов здесь будут с трудом проходить проверку сложности, некоторые выглядят> 10.

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

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

Билл К
источник
1

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

У него было:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

Поэтому я понял, что будет работать следующее:

if(BuildList(list)) 
    SortList(list)

Но не так ясно.

Итак, на оригинальный вопрос, почему бы не иметь:

BuildList(list)
SortList(list)

И пусть SortList решит, требуется ли сортировка?

Вы видите, что ваш метод BuildList, кажется, знает о сортировке, так как он возвращает bool, указывающий как таковой, но это не имеет смысла для метода, предназначенного для построения списка.

Ян
источник
И, конечно, следующий шаг - спросить, почему это двухэтапный процесс. Везде, где я вижу подобный код, я рефакторинг для метода с именем BuildAndSortList (список)
Ян
Это не ответ. Вы изменили поведение кода.
D Drmmr
На самом деле, нет. Опять же, я не могу поверить, что я отвечаю на что-то, что я опубликовал 7 лет назад, но какого черта :) Я спорил о том, что SortList будет содержать условные выражения. Если бы у вас был модульный тест, который утверждал, что список был отсортирован только при соблюдении условия x, он все равно прошел бы. Перемещая условное выражение в SortList, вы избегаете необходимости всегда писать (если (что-то), то SortList (...))
Ян
0

Да, это кажется проблемой, потому что вы должны отслеживать все места, где вы отмечаете флаг ВКЛ / ВЫКЛ. Лучше включать логику только внутри как вложенное условие if, а не выводить логику.

Также богатые доменные модели, в этом случае только один лайнер будет делать большие вещи внутри объекта.

java_mouse
источник
0

Если флаг установлен только один раз, переместите код
...
сразу после
... // какого-то другого кода, а
затем уберите флаг.

В противном случае разделите
... // много кода здесь
... // некоторого другого кода здесь
...
код, если это возможно, на отдельные функции, поэтому ясно, что эта функция несет одну ответственность - логику ветвления.

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

Сам код, а не анти-паттерн, который происходит здесь ... Я подозреваю, что смешивание логики ветвления с фактическим выполнением вещей (команд) - это анти-паттерн, который вы ищете.

Эндрю Пэйт
источник
что этот пост добавляет, что существующие ответы отсутствуют?
esoterik
@esoterik Иногда возможность добавить немного CQRS часто упускается из виду, когда дело доходит до флагов ... логика принятия решения об изменении флага представляет собой запрос, тогда как выполнение работы представляет собой команду. Иногда их разделение может сделать код более понятным. Также в вышеприведенном коде стоило указать, что его можно упростить, поскольку флаг устанавливается только в одной ветви. Я чувствую, что флаги не являются антипаттерном, и если их имя на самом деле делает код более выразительным, это хорошая вещь. Я чувствую, где флаги создаются, устанавливаются и используются должны быть близко друг к другу в коде, если это возможно.
Эндрю Пэйт
0

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

// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(valuesExist) {
    try {
      // Attempt insertion
      trx.commit();
    } catch (DatabaseException dbe) {
      trx.rollback();
      throw dbe;
    }
  } else {
    closeConnection(db);
    throwException();
  }
} else {
  closeConnection(db);
  throwException();
}

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

boolean proceed = true;
// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(!valuesExist) {
    proceed = false;
  }
} else {
  proceed = false;
}

// The moment of truth
if(proceed) {
  try {
    // Attempt insertion
    trx.commit();
  } catch (DatabaseException dbe) {
    trx.rollback();
    throw dbe;
  }
} else {
  if(db.isOpen()) {
    closeConnection(db);
  }
  throwException();
}

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

Нил
источник
В приведенном вами примере, я думаю, я бы предпочел иметь функцию shouldIDoIt (fieldsValidated, valuesExist), которая возвращает ответ. Это связано с тем, что определение «да / нет» делается сразу, в отличие от кода, который я вижу здесь, на работе, где решение продолжить разбросано по нескольким различным несмежным точкам.
Крикет
@KelseyRider, в этом была суть. Отделение проверки от выполнения позволяет вам вставить логику в метод, чтобы упростить общую логику программы в if (isValidated ()) doOperation ()
Нейл
0

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

Теперь ваш конкретный пример можно переписать так:

if(cond1)
{
    ... // lots of code here
    ... // some other code here
    if (cond2)
    {
        ...
    }
}

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

D Drmmr
источник
-1

Локальные флаги, используемые для потока управления, должны распознаваться как скрытая форма goto. Если флаг используется только внутри функции, его можно устранить, написав две копии функции, пометив одну как «flag is true», а другую как «flag is false» и заменив каждую операцию, которая устанавливает флаг когда он очищен, или очищает его, когда он установлен, с переходом между двумя версиями функции.

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

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

Supercat
источник