Inventory check with too many else-ifs

Question

I am using if else statements:

public ActionResult Process(int? inventoryCheck, int? POCheck, int? PromotionCheck)
{
    //var result = db.ValidInventory(2);

    //1. When inventory is low:  
    if (inventoryCheck != null && POCheck == null && PromotionCheck == null)
    {
        db.ValidInventory(inventoryCheck);
    }
    //2. When PO is not generated 
    else if (POCheck != null && inventoryCheck == null && PromotionCheck == null)
    {
        db.ValidInventory(POCheck);
    }
    //3. For Promotion
    else if (POCheck == null && inventoryCheck == null && PromotionCheck != null)
    {
        db.ValidInventory(PromotionCheck);
    }
    else if (inventoryCheck != null && POCheck != null && PromotionCheck == null)
    {
        //4. When Inventory is low and PO is not generated 
        inventoryCheck = 4;
        db.ValidInventory(inventoryCheck);
    }
    else if (inventoryCheck != null && POCheck == null && PromotionCheck != null)
    {
        //5.When inventory is low and Promotion Check 
        inventoryCheck = 5;
        db.ValidInventory(inventoryCheck);
    }
    else if (inventoryCheck == null && POCheck != null && PromotionCheck != null)
    {
        //6. When PO is not generated and Promotion Check  
        inventoryCheck = 6;
        db.ValidInventory(inventoryCheck);
    }
    else if (inventoryCheck != null && POCheck != null && PromotionCheck != null)
    {
        //7. When PO is not generated and Promotion Check   and inventory is low 
        inventoryCheck = 6;
        db.ValidInventory(inventoryCheck);
    }

    return RedirectToAction("BackAnalysis");
}

Here more conditions are present, then I have to add more if else blocks.

How can I optimize it?

Answer 1

You should have to split your function absolutely. You should have to group commons conditions statements and instructions.

However, I think there are deeper problem in your architecture which cause this piece of code. You have to rethink your problem at higher level.

For the three top conditions you can do this, for example :

    public static void Test(int? inventoryCheck, int? POCheck, int? PromotionCheck)
    {
        bool bInventoryCheck = (inventoryCheck == null);
        bool bPOCheck = (POCheck == null);
        bool bPromotionCheck = (PromotionCheck == null);

        ValidInventory (!bInventoryCheck && bPOCheck && bPromotionCheck, inventoryCheck);
        ValidInventory (bInventoryCheck && !bPOCheck && bPromotionCheck, PromotionCheck);
        ValidInventory (bInventoryCheck && bPOCheck && !bPromotionCheck, POCheck);



    }

    public static void ValidInventory(bool validate, int? toValidate)
    {
        if (validate)
            //db.ValidInventory(toValidate);
    }

But I don't think it's a good solution because as you mention here, you have a lot of other cases. We can reduce your code by many way but it's bury the real problem.

In order to reduce we have to understand the concept behind the Process.

PS : Here the "else if" statement seems to be unnecessary, because you are testing every parameters in each case.

Answer 2

I would use something like this:

class Checker {
    private const byte INVENTORY = 1;
    private const byte PO = 2;
    private const byte PROMOTION = 4;

    public void process(int? inventory, int? po, int? promotion){
        int value = (inventory != null ? INVENTORY : 0) +
                    (po != null ? PO : 0) +
                    (promotion != null ? PROMOTION : 0);
        Process(value);
    }

    private void Process(int action){
        switch(action){
            case 0:
                Console.WriteLine("Nothing happens");
                break;
            case INVENTORY:
                Console.WriteLine("inventory check");
                break;
            case PO:
                Console.WriteLine("po check");
                break;
            case PROMOTION:
                Console.WriteLine("promotion check");
                break;
            case INVENTORY + PO:
                Console.WriteLine("inventorycheck = 4 & inventorycheck");
                break;

            // All other statements

            default: 
                throw new ArgumentException("out of bounds");
        }
    }
}

Basically it will determine the action based on a value computed by the combination of the bits representing them. It keeps it very clear what the options are and how they are handled.

An example of usage:

new Checker().process(5, null, null); // "inventory check"
new Checker().process(5, 2, null); // "inventorycheck = 4 & inventorycheck"

---

Should you recognize this approach: https://stackoverflow.com/a/24042179/1864167