Рефакторинг сложного if-условия - PullRequest
Новая
       27

Рефакторинг сложного if-условия

7 голосов
/ 05 мая 2010

Кто-нибудь может предложить лучший способ избежать большинства условий? У меня есть код ниже, я хочу избежать большинства случаев, если условия, как это сделать? любое решение - отличная помощь; 

if (adjustment.adjustmentAccount.isIncrease) {
    if (adjustment.increaseVATLine) {
        if (adjustment.vatItem.isSalesType) {
            entry2.setDebit(adjustment.total);
            entry2.setCredit(0d);
        } else {
            entry2.setCredit(adjustment.total);
            entry2.setDebit(0d);
        }
    } else {
        if (adjustment.vatItem.isSalesType) {
            entry2.setCredit(adjustment.total);
            entry2.setDebit(0d);
        } else {
            entry2.setDebit(adjustment.total);
            entry2.setCredit(0d);
        }
    }
} else {
    if (adjustment.increaseVATLine) {
        if (adjustment.vatItem.isSalesType) {
            entry2.setCredit(adjustment.total);
            entry2.setDebit(0d);
        } else {
            entry2.setDebit(adjustment.total);
            entry2.setCredit(0d);
        }
    } else {
        if (adjustment.vatItem.isSalesType) {
            entry2.setDebit(adjustment.total);
            entry2.setCredit(0d);
        } else {
            entry2.setCredit(adjustment.total);
            entry2.setDebit(0d);
        }
    }
}

Ответы [ 10 ]

18 голосов
/ 05 мая 2010

Как это сделать ... Давайте выделим пару методов, чтобы лучше видеть логику. 

private void a() {
    entry2.setDebit(adjustment.total);
    entry2.setCredit(0d);
}
private void b() {
    entry2.setCredit(adjustment.total);
    entry2.setDebit(0d);
}

if (adjustment.adjustmentAccount.isIncrease) {
    if (adjustment.increaseVATLine) {
        if (adjustment.vatItem.isSalesType) {
            a();
        } else {
            b();
        }
    } else {
        if (adjustment.vatItem.isSalesType) {
            b();
        } else {
            a();
        }
    }
} else {
    if (adjustment.increaseVATLine) {
        if (adjustment.vatItem.isSalesType) {
            b();
        } else {
            a();
    }
} else {
    if (adjustment.vatItem.isSalesType) {
        a();
    } else {
        b();
    }
}

Итак, теперь, глядя на это, этот первый блок 

if (adjustment.increaseVATLine) {
    if (adjustment.vatItem.isSalesType) {
        a();
    } else {
        b();
    }
} else {
    if (adjustment.vatItem.isSalesType) {
        b();
    } else {
        a();
    }
}

равносильно выполнению a(), если adjustment.increaseVATLine имеет то же значение, что и adjustment.vatItem.isSalesType, b() в противном случае. Таким образом, мы можем уменьшить его: 

if (adjustment.adjustmentAccount.isIncrease) {
    if (adjustment.increaseVATLine == adjustment.vatItem.isSalesType) {
        a();
    } else {
        b();
    }
} else {
    if (adjustment.increaseVATLine) {
        if (adjustment.vatItem.isSalesType) {
            b();
        } else {
            a();
        }
    } else {
        if (adjustment.vatItem.isSalesType) {
            a();
        } else {
            b();
        }
    }
}

А оставшийся блок такой же, просто поменять местами a() и b(): 

if (adjustment.adjustmentAccount.isIncrease) {
    if (adjustment.increaseVATLine == adjustment.vatItem.isSalesType) {
        a();
    } else {
        b();
    }
} else {
    if (adjustment.increaseVATLine == adjustment.vatItem.isSalesType) {
        b();
    } else {
        a();
    }
}

Итак, мы начинаем видеть логику. Если это увеличение, а увеличениеVATLine соответствует isSalesType, то мы дебетуем, в противном случае кредитуем, но если это уменьшение, то мы кредитуем только в том случае, если они не совпадают. Какой хороший способ выразить это? Ну, например, назовите a () и b () умнее - теперь, когда мы можем видеть, что они делают 

if (adjustment.adjustmentAccount.isIncrease) {
    if (adjustment.increaseVATLine == adjustment.vatItem.isSalesType) {
        debitEntry();
    } else {
        creditEntry();
    }
} else {
    if (adjustment.increaseVATLine == adjustment.vatItem.isSalesType) {
        creditEntry();
    } else {
        debitEntry();
    }
}

А теперь все еще немного яснее. Дебетовать счет, если это счет увеличения и строка НДС с повышением, а также тип продаж, или когда это уменьшение, либо это строка с уменьшением НДС, ИЛИ это тип продажи, но не оба. Эта таблица истинности помогает? Первый столбец adjustmentAmount.isIncrease; второй adjustment.increaseVATLine; третий adjustment.vatItem.isSalesType. Четвертый столбец - D для дебета, C для кредита; в скобках указано количество значений ИСТИНА среди флагов. 

TTT -> D (3) 
TFF -> D (1) 
TTF -> C (2)
TFT -> C (2) 
FTT -> C (2) 
FFF -> C (0)
FTF -> D (1) 
FFT -> D (1)

Теперь вы можете понять, почему работает решение @Xavier Ho; нечетные суммы - все дебеты, четные - все кредиты.

Это всего лишь один исследовательский путь; Я надеюсь, что это полезно.

8 голосов
/ 05 мая 2010

Я не полностью проверил логику, но это основная идея: 

amt = adjustment.total
if (adjustment.adjustmentAccount.isIncrease
    ^ adjustment.increaseVATLine
    ^ adjustment.vatItem.isSalesType)
{
    amt = -amt;
}

entry2.setCredit(amt > 0 ? amt : 0);
entry2.setDebit(amt < 0 ? -amt : 0);

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

6 голосов
/ 05 мая 2010

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

Редактировать: Я хотел бы отметить из комментариев к этому посту, что решение XOR, предоставленное Marcelo и BlueRaja, идентично по функции. 

/* This is to avoid a crazy 3-way switch. Generalised.
 * Instead of using a complicated if-else branch, we can use the number of true
 * and false to entail the intended action. */
/* This is the same as a ^ b ^ c (chained XOR), 
 * which is used to count the parity of truth values. */
int a = adjustment.adjustmentAccount.isIncrease ? 1 : 0;
int b = adjustment.increaseVATLine ? 1 : 0;
int c = adjustment.vatItem.isSalesType ? 1 : 0;

if ((a + b + c) % 2 == 1)
{
    entry2.setDebit(adjustment.total);          // Odd number of trues
    entry2.setCredit(0d);
}
else
{
    entry2.setCredit(adjustment.total);         // Even number of trues
    entry2.setDebit(0d);
}
6 голосов
/ 05 мая 2010

Вы можете использовать таблицу истинности так: 

debit = ((isIncrease && increaseVATLine && !isSalesType) ||
         (isIncrease && !increaseVATLine && isSalesType) ||
         (!isIncrease && increaseVATLine && isSalesType) ||
         (!isIncrease && !increaseVATLine && !isSalesType)) ? 0 : adjustment.total;
entry2.setCredit(debit);

Нет никаких ifs вообще, и вы можете легко увидеть, в каких случаях дебет равен 0. То же самое для кредита.

5 голосов
/ 05 мая 2010

К комментарию Мартина Смита я добавлю:

Помните, Карно может помочь вам упростить условие, если.

4 голосов
/ 06 мая 2010

На вопрос уже получен ответ, но я опубликую его здесь для тех, кто ищет более чистое решение: 

//Set debit if exactly one or all three are true, else set credit
if(adjustment.adjustmentAccount.isIncrease ^ adjustment.increaseVATLine ^
   adjustment.vatItem.isSalesType)
{
    entry2.setDebit(adjustment.total);
    entry2.setCredit(0d);
}
else
{
    entry2.setCredit(adjustment.total);
    entry2.setDebit(0d);
}
3 голосов
/ 05 мая 2010

Похоже, что у вас есть только 2 случая, чтобы вы могли объединить их с ИЛИ И т.д. и т.п. 

        if (<case1expression>) {
            entry2.setCredit(adjustment.total);
            entry2.setDebit(0d);
        } else {
            entry2.setDebit(adjustment.total);
            entry2.setCredit(0d);
        }
1 голос
/ 06 мая 2010

Лучшее решение - следовать шаблону дизайна. Шаблон проектирования на основе состояния определяет класс для каждого состояния.

Класс состояния затем инкапсулирует, каков курс действий для этого конкретного состояния. Это не только предотвращает большой мусор сетки операторов if -else.

1 голос
/ 05 мая 2010

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

Если, например, у вас есть два класса Increase и NonIncrease, которые являются подклассами одного и того же суперкласса, у вас может быть метод doSomething, который делает - ну - что-то в соответствии с тем классом, который у вас сейчас есть. После этого вам не нужно проверять, «является ли объект do X», а просто вызывать .doSomething (), и он делает все, что должен.

Затем вы можете пойти дальше и иметь все больше и больше подклассов для дальнейшего «уточнения» и «избегания большего количества ifs».

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

1 голос
/ 05 мая 2010

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

...