views:

299

answers:

8

What are the good ways to handle complicated business logic that from the first glance requires many nested if statements?

Example:

Discount Coupon. could be:

1a) Value discount
1b) Percentage discount

2a) Normal discount
2b) Progressive discount

3a) Requires access coupon
3b) Do not require access coupon

4a) Applied only to the customer who already bought before
4b) Applied to any customer

5a) Applied to customer only from countries (X,Y,…)

That requires code even more complicated then this:

if (discount.isPercentage) {
    if (discount.isNormal) {
        if (discount.requiresAccessCoupon) {
        } else {
        }
    } else if (discount.isProgressive) {
        if (discount.requiresAccessCoupon) {
        } else {
        }
    }
} else if (discount.isValue) {
    if (discount.isNormal) {
        if (discount.requiresAccessCoupon) {
        } else {
        }
    } else if (discount.isProgressive) {
        if (discount.requiresAccessCoupon) {
        } else {
        }
    }
} else if (discount.isXXX) {
    if (discount.isNormal) {
    } else if (discount.isProgressive) {
    }
}

Even if you replace IFs to switch/case it's still too complicated. What are the ways to make it readable, maintainable, more testable and easy to understand?

+8  A: 

Specification pattern might be what you are looking for.

Summary:

In computer programming, the specification pattern is a particular software design pattern, whereby business logic can be recombined by chaining the business logic together using boolean logic.

Arnis L.
+4  A: 

I would write a generic state-machine that feeds on lists of things to compare.

jldupont
+2  A: 

The object oriented way of doing it is to have multiple discount classes implementing a common interface:

dicsount.apply(order)

Put the logic for determining whether the order qualifies for the discount within the discount classes.

Scott Saunders
It's not different discount classes. It's one discount class with all those 1-5 things as possible properties.
I'm suggesting that you change that so that you DO have multiple discount classes.
Scott Saunders
Ok, how then specify that discount should be 1a, 2b, 3a, 4b at the same time?
Right, Scott is describing polymorphism. Very important.
Ewan Todd
A: 

My first thought is that this is not testable, which leads me to a solution, in order to get it testable.

if (discount.isPercentage) {
  callFunctionOne(...);
} else if (discount.isValue) {
  callFunctionThree(...);
} else if (discount.isXXX) {
  callFunctionTwo(...);
}

Then you can have each nested if statement be a separate call. This way you can test them individually and when you test the large group you know that each individual one works.

James Black
This is like it's implemented now, still CallFunctionX() requires many IFs inside it and so on...
You may want to evaluate your logic. For example, if you can group the functionality into subclasses then you can see if this discount applies to a subclass and pass it on there.
James Black
A: 

Make methods that checks for a particular case.

bool IsValueNormalAndRequiresCoopon(Discount discount){...}

bool IsValueNormalAndRequiresCoupon(Discount discount){...}

etc

Once you start doing that it becomes easier to see where you can abstract out common logic between the choices. You can then go from there.

For complex decisions I often end up with a class that handles the possible states.

ElGringoGrande
making ~32 such functions for all combination of properties is not so good too
By the time you get some of them done you should start seeing possibilities for making common checks and re-factoring the code to simpler structure.The idea is to remove some of the trees so you can start to view the forest.
ElGringoGrande
+1  A: 

Using guard clauses might help some.

Brian Gideon
But doesn't this violate the single exit principle?
Ewan Todd
Definitely. Though I do not know how much significance that rule has anymore. I certainly do not go out of my way to follow it. Even if you do subscribe to it I think guard clauses would be an acceptable exception.
Brian Gideon
guard clauses will require one new function per each possible combination of 5 different Discount Coupon properties (Volume/Percentage, Normal/Progressive, RequireCoupon/DoNotRequireCoupon,...) that is also not good
+6  A: 

Good question. "Conditional Complexity" is a code smell. Polymorphism is your friend.

Conditional logic is innocent in its infancy, when it’s simple to understand and contained within a few lines of code. Unfortunately, it rarely ages well. You implement several new features and suddenly your conditional logic becomes complicated and expansive. [Joshua Kerevsky: Refactoring to Patterns]

One of the simplest things you can do to avoid nested if blocks is to learn to use Guard Clauses.

double getPayAmount() {
if (_isDead) return deadAmount();
if (_isSeparated) return separatedAmount();
if (_isRetired) return retiredAmount();
return normalPayAmount();
};

The other thing I have found simplifies things pretty well, and which makes your code self-documenting, is Consolidating conditionals.

double disabilityAmount() {
 if (isNotEligableForDisability()) return 0;
 // compute the disability amount

Other valuable refactoring techniques associated with conditional expressions include Decompose Conditional, Replace Conditional with Visitor, and Reverse Conditional.

Ewan Todd
+1 - I like the way you gave different options.
James Black
Looks like "Guard Clauses" solution will work only if comparing attributes are self-excludive (_isDead, _isSeparated, isRetired) it's not possible to use this solution for provided example, because Discount Coupon could be both "1a) Value discount, 2a) Normal discount, 3b) Do not require access coupon, 4a) Applied only to the customer who already bought before and 5a) Applied to customer only from countries (X,Y,…)"
or add near 32 such return statements:_isValueAndNoramAndAcees()_isValueAndProgressiveAndNotRequireAccessAndRequireCountires().... all other combinations in IF and and in function
A: 

FWIW, I have used Hamcrest very successfully for this sort of thing. I believe you could say that it implements the Specification Pattern, @Arnis talked about.

David Wolever