Refactoring a complicated if-condition

Question

Hi all,

Can anyone suggest best way to avoid most if conditions? I have below code, I want avoid most of cases if conditions, how to do it ? any solution is great help;

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);
        }
    }
}

Answer 1

If you have conditional logic (e.g. do something if a condition is met), why would you want to even try and avoid them?

Answer 2

It looks like you only have 2 cases so you could combine them with OR AND etc.

        if (<case1expression>) {
            entry2.setCredit(adjustment.total);
            entry2.setDebit(0d);
        } else {
            entry2.setDebit(adjustment.total);
            entry2.setCredit(0d);
        }

Answer 3

I haven't thoroughly verified the logic, but this is the basic idea:

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);

I should note that this logic is slightly different in that it correctly handles a negative value of adjustment.total, whereas the original seems to assume (perhaps correctly) that the value will always be non-negative.

Answer 4

What you usually can do to ease the situation to some extent is use inheritance.

If you for example have two classes Increase and NonIncrease that are subclasses of the same superclass you can have a method doSomething that does - well - something according to whatever class you currently have. You then do not have to check "if object is do X" but just call .doSomething() and it does whatever it is meant to do.

You can then go further and have more and more subclasses to further "refine" and "avoid more ifs".

There might be other possibilities (largely depending on your environment/requirements) like function pointers, delegates, the strategy pattern (GoF) or such constructs.

Answer 5

You could use a truth table like this:

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

There are no ifs whatsoever and you can see easily in which cases the debit is 0. Same thing for the credit.

Answer 6

To Martin Smith comment I'll add:

Remember, Karnaugh can help you to simplify the condition of that if.

Answer 7

I think this works. I basically generalised your boolean logic. Next time, try drawing some diagrams to help clear your mind.

Edit: I'd like to point out from the comments to this post, that the XOR solution provided by Marcelo and BlueRaja is identical in function.

/* 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);
}

Answer 8

How to go about this... Let's extract a couple of methods, so we can better see the logic.

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();
    }
}

So now, looking at it, that first block

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

just amounts to doing a() if adjustment.increaseVATLine has the same value as adjustment.vatItem.isSalesType, b() otherwise. So we can reduce it:

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();
        }
    }
}

And the remaining block is the same, just reversing a() and b():

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

So we begin to see the logic. If it's an increase, and the increaseVATLine matches the isSalesType, then we debit, otherwise credit, but if it's a decrease, then we credit only if they don't match. What's a good way of expressing this? Well, for one, name a() and b() smarter - now that we can see what they're doing

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

And now it's a little clearer still. Debit the account when it's an increase account and an increase VAT line, and a sales type, or when it's a decrease and either it's a decrease VAT line OR it's a sales type, but not both. Does this truth table help? First column is adjustmentAmount.isIncrease; second is adjustment.increaseVATLine; third is adjustment.vatItem.isSalesType. Fourth column is D for debit, C for credit; in parentheses are the number of TRUE values among the flags.

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

Now you can see why @Xavier Ho's solution works; the odd totals are all debits, the even ones all credits.

This is just one exploratory path; I hope it's helpful.

Answer 9

The best solution is to follow a design pattern. A State based design pattern defines a class per each state.

The state class then encapsulates what is the course of action for that particular state. This not only prevents a large junk of if -else statement mesh.

Answer 10

Question has been answered, but I'll post there here for those who care for a cleaner solution:

//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);
}