Как мне отредактировать цепочку операторов if-else if в соответствии с принципами чистого кода дяди Боба?

45

Я пытаюсь следовать рекомендациям дяди Боба по чистому коду и, в частности, держать методы короткими

Я не могу сократить эту логику, хотя:

if (checkCondition()) {addAlert(1);}
else if (checkCondition2()) {addAlert(2);}
else if (checkCondition3()) {addAlert(3);}
else if (checkCondition4()) {addAlert(4);}

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

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


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

Я считаю, что это другой вопрос (вы можете увидеть это также, сравнив ответы на эти вопросы).

  • Мой вопрос проверяет, чтобы первое принятое условие закончилось быстро .
  • Связанный вопрос пытается создать все условия, чтобы принять что-то. (лучше видно в этом ответе на этот вопрос: https://softwareengineering.stackexchange.com/a/122625/96955 )
Ev0oD
источник
46
Что на самом деле неправильно или неясно об этом коде в его контексте? Я не вижу, как это может быть сокращено или упрощено! Код, который оценивает условия, уже выглядит хорошо продуманным, как и метод, который вызывается в результате решения. Вам нужно только взглянуть на некоторые из приведенных ниже ответов, которые просто усложняют код!
Стив
38
В этом коде нет ничего плохого. Это очень читабельно и легко следовать. Все, что вы делаете, чтобы уменьшить его, добавит косвенности и усложнит понимание.
17 из 26
20
Ваш код в порядке. Вложите оставшуюся энергию во что-то более продуктивное, чем пытайтесь сократить ее дальше.
Роберт Харви
5
Если это действительно только 4 условия, это нормально. Если это действительно что-то вроде 12 или 50, то вы, вероятно, хотите провести рефакторинг на более высоком уровне, чем этот метод.
JimmyJames
9
Оставьте свой код точно таким, как есть. Послушайте, что ваши родители всегда говорили вам: не доверяйте ни одному дяде, предлагающему сладости детям на улице. @ Харви Забавно, но различные попытки «улучшить» код сделали его намного больше, сложнее и менее читабельным.
gnasher729

Ответы:

81

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

{
    addAlert(GetConditionCode());
}

и у вас есть GetConditionCode () инкапсулировать логику для проверки условий. Возможно также лучше использовать Enum, чем магическое число.

private AlertCode GetConditionCode() {
    if (CheckCondition1()) return AlertCode.OnFire;
    if (CheckCondition2()) return AlertCode.PlagueOfBees;
    if (CheckCondition3()) return AlertCode.Godzilla;
    if (CheckCondition4()) return AlertCode.ZombieSharkNado;
    return AlertCode.None;
}
Стивен Эклс
источник
2
Если возможно инкапсулировать, как вы описываете (я подозреваю, что это может быть не так, я думаю, что OP упускает переменные для простоты), это не меняет код, что само по себе хорошо, но добавляет эргономику кода и немного читабельности + 1
опа
17
С этими кодами оповещения, я благодарю за код, только один может быть возвращен за один раз
Josh Part
12
Кажется, это также идеально подходит для использования оператора switch - если это доступно на языке OP.
Фрэнк Хопкинс
4
Вероятно, хорошей идеей будет извлечь получение кода ошибки в новый метод, если он может быть записан так, чтобы он был полезен во многих ситуациях, без необходимости получать кучу информации о конкретной ситуации. На самом деле есть компромисс и точка безубыточности, когда это того стоит. Но достаточно часто вы увидите, что последовательность проверок зависит от конкретной работы, и лучше держать их вместе с этой работой. В таких случаях создание нового типа для сообщения другой части кода о том, что необходимо сделать, является нежелательным балластом.
PJTraill
6
Одна проблема с этим переопределением состоит в том, что он заставляет функцию addAlertпроверять наличие фиктивного состояния предупреждения AlertCode.None.
Дэвид Хаммен
69

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

Любая попытка дальнейшего «упрощения» действительно усложнит ситуацию.

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


В стороне:

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

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

cmaster
источник
2
Я бы сказал, что замена else ifвнутреннего с returnпоследующим простым if(удаление else) может сделать код более трудным для чтения . Когда код говорит else if, я сразу знаю, что код в следующем блоке будет выполняться только в том случае, если предыдущий не выполнял. Нет суеты, нет суеты. Если это равнина, ifто он может выполняться или не выполняться, независимо от того, был ли выполнен предыдущий. Теперь мне придется потратить некоторое количество умственных усилий, чтобы разобрать предыдущий блок, чтобы отметить, что он заканчивается на return. Я бы лучше потратил эти умственные усилия на разбор бизнес-логики.
CVN
1
Я знаю, это мелочь, но, по крайней мере, для меня, это else ifодна семантическая единица. (Это не обязательно отдельный модуль для компилятора, но это нормально.) ...; return; } if (...Не; не говоря уже о том, если он разбит на несколько строк. Это то, на что я действительно должен смотреть, чтобы увидеть, что он делает, вместо того, чтобы иметь возможность принять это непосредственно, просто увидев пару ключевых слов else if.
CVN
@ MichaelKjörling Full Ack. Я бы предпочел саму else ifконструкцию, тем более что ее цепочечная форма - такой хорошо известный шаблон. Тем не менее, код формы if(...) return ...;также является широко известным шаблоном, поэтому я не буду полностью осуждать это. Однако я считаю это незначительной проблемой: логика потока управления одинакова в обоих случаях, и один более внимательный взгляд на if(...) { ...; return; }лестницу скажет мне, что она действительно эквивалентна else ifлестнице. Я вижу структуру одного термина, я понимаю его значение, я понимаю, что он повторяется везде, и я знаю, что случилось.
начальник
Исходя из JavaScript / node.js, некоторые использовали бы код "пояс и подтяжки", используя и то, else if и другоеreturn . например,else if(...) { return alert();}
user949300
1
«И помни, не будь религиозным даже об этом совете». +1
Слова,
22

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

Поместите ваши условия в объекты и поместите эти объекты в список

foreach(var condition in Conditions.OrderBy(i=>i.OrderToRunIn))
{
    if(condition.EvaluatesToTrue())
    {
        addAlert(condition.Alert);
        break;
    }
}

Если требуется несколько действий при условии, вы можете сделать некоторую сумасшедшую рекурсию

void RunConditionalAction(ConditionalActionSet conditions)
{
    foreach(var condition in conditions.OrderBy(i=>i.OrderToRunIn))
    {
        if(condition.EvaluatesToTrue())
        {
            RunConditionalAction(condition);
            break;
        }
    }
}

Очевидно, да. Это работает, только если у вас есть образец для вашей логики. Если вы попытаетесь выполнить суперобобщенное рекурсивное условное действие, то настройка для объекта будет такой же сложной, как и оригинальная инструкция if. Вы будете изобретать свой новый язык / рамки.

Но ваш пример действительно есть шаблон

Распространенным вариантом использования этого шаблона будет проверка. Вместо :

bool IsValid()
{
    if(condition1 == false)
    {
        throw new ValidationException("condition1 is wrong!");
    }
    elseif(condition2 == false)
    {
    ....

}

становится

[MustHaveCondition1]
[MustHaveCondition2]
public myObject()
{
    [MustMatchRegExCondition("xyz")]
    public string myProperty {get;set;}
    public bool IsValid()
    {
        conditions = getConditionsFromReflection()
        //loop through conditions
    }
}
Ewan
источник
27
Это только перемещает if...elseлестницу в построении Conditionsсписка. Чистый выигрыш отрицателен, так как построение Conditionsзаймет столько же кода, сколько и код OP, но добавленная косвенность приводит к затратам на читабельность. Я бы определенно предпочел чистую кодовую лестницу.
Мастер
3
@cmaster да, я думаю, что я точно сказал, что «тогда установка для объекта будет такой же сложной, как и оригинальная инструкция if»
Ewan
7
Это менее читабельно, чем оригинал. Чтобы выяснить, какое условие на самом деле проверяется, вам нужно копаться в какой-то другой части кода. Это добавляет ненужный уровень косвенности, который затрудняет понимание кода.
17 из 26
8
Преобразование цепочки if .. else if .. else .. в таблицу предикатов и действий имеет смысл, но только для гораздо более крупных примеров. Таблица добавляет некоторую сложность и косвенность, поэтому вам нужно достаточно записей для амортизации этих концептуальных накладных расходов. Таким образом, для 4 пар предикат / действие сохраните простой исходный код, но если у вас есть 100, обязательно используйте таблицу. Точка кроссовера находится где-то посередине. @cmaster, таблица может быть статически инициализирована, поэтому дополнительные затраты на добавление пары предикат / действие - это одна строка, которая просто называет их: трудно сделать лучше.
Стивен С. Сталь
2
Читаемость не является личной. Это обязанность программистов. Это субъективно. Именно поэтому важно приходить в такие места и слушать, что публика о них говорит. Лично я считаю этот пример неполным. Покажи мне, как conditionsустроено ... АРГ! Не аннотации-атрибуты! Господи, почему? О, мои глаза!
Candied_Orange
7

Подумайте об использовании return;после выполнения одного условия, оно спасет вас от всех elseошибок. Вы могли бы даже иметь возможность return addAlert(1)напрямую, если этот метод имеет возвращаемое значение.

Килиан Фот
источник
3
Конечно, это предполагает, что больше ничего не происходит после цепочки ifс ... Это может быть разумным предположением, а с другой стороны, это может и не произойти.
CVN
5

Я видел такие конструкции, которые иногда считаются более чистыми:

switch(true) {
    case cond1(): 
        statement1; break;
    case cond2():
        statement2; break;
    case cond3():
        statement3; break;
    // .. etc
}

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

cond1() ? statement1 :
cond2() ? statement2 :
cond3() ? statement3 : (null);

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

zworek
источник
1
троичный аккуратный
Ewan
6
@ Отладка сломанной «глубоко рекурсивной троицы» может быть ненужной болью.
'15
5
это выглядит аккуратно на экране, хотя.
Эван
Хм, какой язык позволяет использовать функции с caseметками?
Undercat поддерживает Монику
1
@undercat, это действительный ECMAScript / JavaScript afaik
zworek
1

В качестве варианта ответа @ Ewan вы можете создать цепочку (вместо «плоского списка») таких условий:

abstract class Condition {
  private static final  Condition LAST = new Condition(){
     public void alertOrPropagate(DisplayInterface display){
        // do nothing;
     }
  }
  private Condition next = Last;

  public Condition setNext(Condition next){
    this.next = next;
    return this; // fluent API
  }

  public void alertOrPropagate(DisplayInterface display){
     if(isConditionMeet()){
         display.alert(getMessage());
     } else {
       next.alertOrPropagate(display);
     }
  }
  protected abstract boolean isConditionMeet();
  protected abstract String getMessage();  
}

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

Именно здесь он превосходит подход «плоского списка», где необходимо реализовать «пропуск» в цикле, который применяет условия.

Вы просто настраиваете цепочку условий:

Condition c1 = new Condition1().setNext(
  new Condition2().setNext(
   new Condition3()
 )
);

И начните оценку с простого вызова:

c1.alertOrPropagate(display);
Тимоти Тракл
источник
Да, это называется Образцом цепочки ответственности
Макс
4
Я не буду притворяться, что говорю за кого-то еще, но хотя код в вопросе сразу читается и очевиден в своем поведении, я не считаю это сразу очевидным в отношении того, что он делает.
CVN
0

Прежде всего, оригинальный код не страшен IMO. Это довольно понятно, и в этом нет ничего плохого.

Тогда, если вам это не нравится, опираясь на идею @ Ewan использовать список, но удаляя его несколько неестественный foreach breakшаблон:

public class conditions
{
    private List<Condition> cList;
    private int position;

    public Condition Head
    {
        get { return cList[position];}
    }

    public bool Next()
    {
        return (position++ < cList.Count);
    }
}


while not conditions.head.check() {
  conditions.next()
}
conditions.head.alert()

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

РЕДАКТИРОВАТЬ: похоже, это не так ясно, как я думал, поэтому позвольте мне объяснить подробнее. conditionsупорядоченный список какого-то рода; headтекущий исследуемый элемент - в начале это первый элемент списка, и при каждом next()вызове он становится следующим; check()и alert()являются checkConditionX()и addAlert(X)от ОП.

Нико
источник
1
(Не понизил, но) Я не могу следовать этому. Что такое голова ?
Belle-Sophie
@Belle Я отредактировал ответ, чтобы объяснить дальше. Это та же идея, что и у Юана, но while notвместо foreach break.
Нико
Блестящая эволюция блестящей идеи
Эван
0

В этом вопросе отсутствует определенность. Если условия:

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

или если содержание в addAlertболее сложное, то, возможно, лучшее решение, скажем, в c # будет:

//in some central spot
IEnumerable<Tuple<Func<bool>, int>> Conditions = new ... {
  Tuple.Create(CheckCondition1, 1),
  Tuple.Create(CheckCondition2, 2),
  ...
}

//at the original place
var matchingCondition = Conditions.Where(c=>c.Item1()).FirstOrDefault();
if(matchingCondition != null) 
  addAlert(matchingCondition.Item2)

Кортежи не так красивы в c # <8, но выбраны для удобства.

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

NiklasJ
источник
0

Лучший способ уменьшить цикломатическую сложность в тех случаях, когда у вас их много, if->then statements- это использовать словарь или список (в зависимости от языка) для хранения значения ключа (если значение оператора или какое-либо значение), а затем значения / результата функции.

Например, вместо (C #):

if (i > 10) { return "Two"; }
else if (i > 8) { return "Four" }
else if (i > 4) { return "Eight" }
return "Ten";  //etc etc say anything after 3 or 4 values

Я могу просто

var results = new Dictionary<int, string>
{
  { 10, "Two" },
  { 8, "Four"},
  { 4, "Eight"},
  { 0, "Ten"},
}

foreach(var key in results.Keys)
{
  if (i > results[key]) return results.Values[key];
}

Если вы используете более современные языки, вы можете хранить больше логики, чем просто значения (c #). На самом деле это просто встроенные функции, но вы также можете просто указать на другие функции, если логика должна быть просто встроенной.

var results = new Dictionary<Func<int, bool>, Func<int, string>>
{
  { (i) => return i > 10; ,
    (i) => return i.ToString() },
  // etc
};

foreach(var key in results.Keys)
{ 
  if (key(i)) return results.Values[key](i);
}
Эрик Филипс
источник
0

Я пытаюсь следовать рекомендациям дяди Боба по чистому коду и, в частности, держать методы короткими

Я не могу сократить эту логику, хотя:

if (checkCondition()) {addAlert(1);}
else if (checkCondition2()) {addAlert(2);}
else if (checkCondition3()) {addAlert(3);}
else if (checkCondition4()) {addAlert(4);}

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

if (is_an_apple()) {
  addAlert(1);
}
else if (is_a_banana()) {
  addAlert(2);
}
else if (is_a_cat()) {
  addAlert(3);
}
else if (is_a_dog()) {
  addAlert(4);
}

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

CJ Деннис
источник
0

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

EG: checkCondition1()стал бы evaluateCondition1(), на котором он будет проверять, было ли выполнено предыдущее условие; если это так, то он кэширует какое-то значение для извлечения getConditionNumber().

checkCondition2()станет evaluateCondition2(), на котором он будет проверять, были ли выполнены предыдущие условия. Если предыдущее условие не было выполнено, выполняется проверка сценария условия 2 с кэшированием значения, которое должно быть получено getConditionNumber(). И так далее.

clearConditions();
evaluateCondition1();
evaluateCondition2();
evaluateCondition3();
evaluateCondition4();
if (anyCondition()) { addAlert(getConditionNumber()); }

РЕДАКТИРОВАТЬ:

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

bool evaluateCondition34() {
    if (!anyCondition() && A && B && C) {
        conditionNumber = 5693;
        return true;
    }
    return false;
}

...

bool evaluateCondition76() {
    if (!anyCondition() && !B && C && D) {
        conditionNumber = 7658;
        return true;
    }
    return false;
}

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

clearConditions();
evaluateCondition10();
evaluateCondition9();
evaluateCondition8();
evaluateCondition7();
...
evaluateCondition34();
...
evaluateCondition76();

if (anyCondition()) { addAlert(getConditionNumber()); }

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

Эмерсон Кардосо
источник
Мне не нравится это предложение - оно скрывает логику тестирования внутри нескольких функций. Это может усложнить поддержку кода, если, например, вам нужно было изменить порядок и выполнить # 3 до # 2.
Лоуренс
Нет. Вы можете проверить, оценивалось ли какое-либо предыдущее условие, если anyCondition() != false.
Эмерсон Кардосо
1
Хорошо, я вижу, к чему ты клонишь. Однако, если (скажем) оба условия 2 и 3 оцениваются как true, OP не хочет, чтобы условие 3 оценивалось.
Лоуренс
Я имел в виду, что вы можете проверить anyCondition() != falseв функциях evaluateConditionXX(). Это возможно реализовать. Если подход с использованием внутреннего состояния нежелателен, я понимаю, но аргумент, что это не работает, недопустим.
Эмерсон Кардосо
1
Да, я не согласен с тем, что он бесполезно скрывает логику тестирования, а не то, что он не может работать. В вашем ответе (параграф 3) проверка на соответствие условию 1 помещена в eval ... 2 (). Но если он переключает условия 1 и 2 на верхнем уровне (из-за изменений в требованиях клиентов и т. Д.), Вам придется перейти в eval ... 2 (), чтобы убрать проверку для условия 1, плюс перейти в eval. ..1 () чтобы добавить проверку для условия 2. Это может быть сделано для работы, но это может легко привести к проблемам с техническим обслуживанием.
Лоуренс
0

Более двух предложений «else» вынуждают читателя кода пройти всю цепочку, чтобы найти интересующую его. Используйте метод, такой как: void AlertUponCondition (Условие условия) {switch (условие) {case Condition.Con1: ... break; case Condition.Con2: ... break; и т. д.}} Где «Condition» - это правильное перечисление. При необходимости верните бул или значение. Назовите это так: AlertOnCondition (GetCondition ());

Это действительно не может быть проще, и это быстрее, чем цепочка if-else, если вы превысите несколько случаев.

Джимми Т
источник
0

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

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

Полиморфизм мог бы подойти вам лучше.

С подозрением относитесь к коду с длинными методами, содержащими длинные или сложные конструкции if-then. Вам часто нужно дерево классов с некоторыми виртуальными методами.

Мартин Маат
источник