A little effort to eliminate complicated if-else

background

When doing code reviews for coding beginners, we often see deep nesting of if-else. Even metric tools such as SonarQube get angry at "cyclomatic complexity is too high".

If you try to reduce conditional branching thoroughly, you will often hear about design patterns such as the Strategy pattern and the State pattern, but even if you don't go that far, the code will be easier to read with a little care. .. For beginners.

Example

Below is the code that determines if the item is available for purchase.

public boolean canPurchase() {
    boolean result;

    //Are you logged in
    if (isLoggedIn()) {
        //Is it in stock?
        if (existsStock()) {
            //In the case of cash on delivery
            if (isPaymentMethodCash()) {
                result = true;
            //For credit card payment
            } else {
                //Is your credit card registered?
                if (isCreditCardRegistered()) {
                    result = true;
                } else {
                    result = false;
                }
            }
        } else {
            result = false;
        }
    } else {
        result = false;
    }

    return result;
}

This is a typical example of deep nesting, and the code is very difficult to follow. Looking at the code of an unfamiliar person, I feel that I am conscious of the normal workflow as shown below.

if (1_Normal system) {
    if (2_Normal system) {
        ...
        ...
        ... //Long processing
        ...
        ...
    } else {
        // 2_Abnormal system
        return null;
    }
} else {
    // 1_Abnormal system
    throw new SomeException();
}

However, in "special" cases such as not logged in or out of stock, other conditions (payment method and card registration) are irrelevant and the return value is unconditionally determined (or exception). Will occur). So let's separate these cases from other logic.

First, let's separate the login conditions.

public boolean canPurchase() {

    //Unconditionally false if you are not logged in
    if (!isLoggedIn()) {
        return false;
    }

    //Is it in stock?
    if (existsStock()) {
        //In the case of cash on delivery
        if (isPaymentMethodCash()) {
            return true;
        //For credit card payment
        } else {
            //Is your credit card registered?
            if (isCreditCardRegistered()) {
                return true;
            } else {
                return false;
            }
        }
    } else {
        return false;
    }
}

In the first code, the local variable result was padded with a boolean value and finally return result, but this time the boolean value is immediately return. It seems that the method of returning when the return value is fixed without using temporary variables like this is called early return. Also, like the above code, the method of making an early return of a special case at the beginning and excluding that special case after that is called a guard clause. This way you don't have to use the else clause and the nesting is gone.

Let's also separate inventory conditions.

public boolean canPurchase() {

    //Unconditionally false if not logged in
    if (!isLoggedIn()) {
        return false;
    }

    //Unconditionally false if out of stock
    if (!existsStock()) {
        return false;
    }

    //In the case of cash on delivery
    if (isPaymentMethodCash()) {
        return true;
    //For credit card payment
    } else {
        //Is your credit card registered?
        if (isCreditCardRegistered()) {
            return true;
        } else {
            return false;
        }
    }
}

I think that the nesting has been eliminated and it is easier to follow the process. It's a good way to get a good view of the code with a little effort, so please keep it in mind.

** * Scope of guard clause ** If you go further here, it will always be true in the case of cash on delivery, so you can further reduce nesting by applying a guard clause here as well.

public boolean canPurchase() {

    //Unconditionally false if not logged in
    if (!isLoggedIn()) {
        return false;
    }

    //Unconditionally false if out of stock
    if (!existsStock()) {
        return false;
    }

    //Unconditionally true for cash on delivery
    if (isPaymentMethodCash()) {
        return true;
    }

    //Is your credit card registered?
    if (isCreditCardRegistered()) {
        return true;
    } else {
        return false;
    }
}

However, this may be a matter of taste or policy, but I personally think that it is better not to do it. The reason is that the guard clause is just a special case in advance, and cash on delivery or credit card payment are both common conditions in normal cases. If the conditions are equal, I think you should arrange them in parallel with if-else. Also, if the order of the guard clauses is changed as shown below, problems will occur.

/**Bad example. True is returned if cash on delivery even if you are not logged in**/
public boolean canPurchase() {

    //Unconditionally true for cash on delivery
    if (isPaymentMethodCash()) {
        return true;
    }

    //Unconditionally false if not logged in
    if (!isLoggedIn()) {
        return false;
    }

    //Unconditionally false if out of stock
    if (!existsStock()) {
        return false;
    }

    //Is your credit card registered?
    if (isCreditCardRegistered()) {
        return true;
    } else {
        return false;
    }
}

I think that the guard clause should be kept within the range that can be judged independently (independent of the order).