Предлагаемый рефакторинг для if-оператора следующей формы? - PullRequest
1 голос
/ 09 апреля 2019

Я пытаюсь сделать это как можно более общим.Давайте предположим, что в операторе if я проверяю, верно ли какое-либо логическое выражение A.Допустим, есть конкретные случаи, когда A является истинным, Aa и Ab, которые являются взаимоисключающими.То же самое верно для другого логического выражения B. Затем рассмотрим следующий код:

if (A) {
  if (A.a) {
    somethingSpecificToAa();
    foo();
  }
} else if (B) {
  if (B.a) {
    somethingSpecificToBa();
    foo();
  }
} else {
    foo();
}

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

Поскольку foo() выполняется, когда:

  • Aa истинно
  • Ba истинно
  • Ни A, ни B не соответствуют действительности

Я подумал о следующем:

if (A.a) {
  somethingSpecificToAa();
} else if (B.a) {
  somethingSpecificToBa();
}

if (A.a || B.a || !(A || B)) {
  foo();
}

, который должен иметь такое же поведение.Это лучший способ сделать это?Обратите внимание, что условие во втором операторе if второго примера в действительности оказывается очень длинным, поэтому мой код по-прежнему выглядит как первый (я ненавижу разбивать один if на несколько строк).подумал о создании лямбда-выражения, которое возвращает bool, что эквивалентно A.a || B.a || !(A || B), и о включении лямбды во 2-й оператор if.В качестве альтернативы, я мог бы сохранить структуру 1-го примера, но заменить множество строк каждого foo() на (void) лямбду, которая делает то же самое, хотя я не уверен, что это устраняет запах.

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

РЕДАКТИРОВАТЬ: Кажется, я сделал это слишком универсальный.Я имею дело с контейнерами STL, а не с моими собственными классами, и более «точным» примером будет:

int shirtACleanliness = calculateCleanliness(shirtA);
if (itemsToWash.contains(shirtA)) { //itemsToWash is a std::set
  if (shirtA.cleanliness > shirtACleanliness) {
    itemsToWash.erase(shirtA);
    shirtA.cleanliness = shirtACleanliness;
    itemsToWash.insert(shirtA); //the set is ordered on cleanliness, so this re-inserts in the correct position
    doSomeOtherStuff(shirtA);
  }
} else if (itemsToDry.contains(shirtA)) { //itemsToDry is a std::vector
  if (shirtA.cleanliness > shirtACleanliness) {
    itemsToDry.erase(shirtA);
    shirtA.cleanliness = shirtACleanliness;
    itemsToWash.insert(shirtA);
    doSomeOtherStuff(shirtA);
  }
} else {
  shirtA.cleanliness = shirtACleanliness;
  itemsToWash.insert(shirtA);
  doSomeOtherStuff(shirtA);
}
//am aware aware contains() is not a method for either type
//and std::vector does not have erase() by value, this is just conceptual

Ответы [ 2 ]

0 голосов
/ 09 апреля 2019

Согласно вашему недавнему комментарию, здесь есть немного псевдокода, который может соответствовать вашему варианту использования.

Element* element=nullptr;

if(vectorA.contains(X)){
   element = vectorA.get(X);
}else if(setB.contains(X)){
   element = setB.get(X);
}

if(element != nullptr && element->a){
   something(element);
}

if(element == nullptr || element->a){
    foo();
}
0 голосов
/ 09 апреля 2019

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

template<typename T, typename F>
bool doSomething(T const& t, F f)
{
    if (t && t.a)
    {
        f();
        foo();
    }

    return static_cast<bool>(t);
}

И использовать это как

if (!doSomething(A, &somethingSpecificToAa) && !doSomething(B, &somethingSpecificToBa))
{
    foo();
}
...