Refactoring a complex if-state

Question

Can anyone suggest a better way to avoid most situations? I have below code, I want to avoid most cases, if conditions, how to do it? any solution is a big 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

I think it works. I basically summarized your logical logic. Next time, try drawing some diagrams to clear your mind.

Edit: I would like to point out from the comments on 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 2

How to do it ... Let me extract a few methods to better understand 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 him, this first block

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

just means executing a() if adjustment.increaseVATLine has the same meaning as adjustment.vatItem.isSalesType , b() otherwise. Therefore, 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 changing 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 are starting to see the logic. If this increase and increase of VATLine corresponds to isSalesType, then we are a debit, otherwise a loan, but if it is a decrease, then we credit only if they do not match. What a good way to express it? Well, for one, call a () and b () smarter - now that we can see what they do

 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. Debit the account when it increases the account and increases the VAT line, as well as the type of sales or when it decreases, and either it decreases the VAT line, or it is the type of sales, but not both. Does this truth table help? First column adjustmentAmount.isIncrease ; the second is adjustment.increaseVATLine ; the third is adjustment.vatItem.isSalesType . The fourth column is D for debit, C for credit; in parentheses indicates 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 understand why @Xavier Ho's solution works; odd totals - all debit, even - all loans.

This is just one research path; Hope this will be helpful.

Answer 3

I did not fully check the logic, but this is the main 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 somewhat different in that it correctly processes the negative adjustment.total value, while the original seems to suggest (possibly correctly) that the value will always be non-negative.

Answer 4

You can use the truth table as follows:

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

There are no ifs, and you can easily see in which cases the debit is 0. The same is for credit.

Answer 5

In Martin Smith's comment, I will add:

Remember, Karnaugh can help you simplify the if if condition.

Answer 6

The question was answered, but I will post there for those who care about 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); } 

Answer 7

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

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

Answer 8

If you have conditional logic (for example, to do something if the condition is met), why do you even want to try to avoid them?

Answer 9

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

If you, for example, have two classes Increase and NonIncrease , which are subclasses of the same superclass, you can have a doSomething method that does - well, something in accordance with the class that you currently have. Then you do not need to check "if the object is X", but simply call .doSomething (), and it does what it should do.

Then you can move on and have more and more subclasses for further “refinement" and "avoid more ifs".

There may be other features (depending largely on your environment / requirements), such as function pointers, delegates, a strategy template (GoF), or such constructs.

Answer 10

The best solution is to follow the design pattern. A state-based design pattern defines a class for each state.

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