Add-to-cart method has too many conditions

Question

I have a method that seems to be too much for one method. I might be validating programming methodology. I just don't know what to do with it. Would a switch statement be better? Should I break it up and provide other methods?

public void addToCart(OrderlineType orderline) throws Exception
{

        if (orderline == null)
        {
            throw new OutOfStockException("Null object pass addToCart");
        }

        if(orderline.getProduct().isRecallStatus())
        {
             throw new ProductRecallException("Product Id: " + orderline.getProduct().getProductId() 
                     + " is being Recalled");
        }

        int tempOrderlineQuantity = orderline.getQuantity();
        int tempOrderlineProductId = orderline.getProduct().getProductId();

        if( Inventory.getInstance().getQuantity(tempOrderlineProductId)
                        == tempOrderlineQuantity  )
          {
                   shoppingCart.add(orderline);
                   log.warn("Product " + orderline.getProduct().getProductName()
                           +" ID: "+ tempOrderlineProductId 
                           + "after purchase will have no more in stock");

          }
        else if(Inventory.getInstance().getQuantity(tempOrderlineProductId)
                        > tempOrderlineQuantity)
          {
                    shoppingCart.add(orderline);
          }
        else if(Inventory.getInstance().getQuantity(tempOrderlineProductId)
                            == -1)

          {
                        throw new OutOfStockException("PrductId: " + tempOrderlineProductId 
                                + " is not listed in the Inventory: "
                                + Inventory.getInstance().getQuantity(tempOrderlineProductId));

           }
         else
          {
                        throw new OutOfStockException("PrductId: " + tempOrderlineProductId 
                                                    + " does not have enough to complete this order, the in Stock amount is: "
                                                    + Inventory.getInstance().getQuantity(tempOrderlineProductId));
          }
}

Answer 1

public void addToCart(OrderlineType orderline) throws Exception
{

        if (orderline == null)
        {
            throw new OutOfStockException("Null object pass addToCart");
        }

You almost certainly don't want to treat a null orderline the same as an OutOfStockException. You probably don't need to check this at all. If a null orderline is illegal, just let the function throw the NullPtrException that results.

        if(orderline.getProduct().isRecallStatus())
        {
             throw new ProductRecallException("Product Id: " + orderline.getProduct().getProductId() 
                     + " is being Recalled");
        }

This isn't really the best place for rules about whether you can sell a product. Instead, I'd suggest:

        orderline.getProduct().verifyAllowedToSell();

Which will throw the exception if you aren't allowed to sell the product because its recalled, or any other reason.

        int tempOrderlineQuantity = orderline.getQuantity();
        int tempOrderlineProductId = orderline.getProduct().getProductId();

Avoid storing variables in temporary locals unless you really need to.

        if( Inventory.getInstance().getQuantity(tempOrderlineProductId)
                        == tempOrderlineQuantity  )
          {
                   shoppingCart.add(orderline);
                   log.warn("Product " + orderline.getProduct().getProductName()
                           +" ID: "+ tempOrderlineProductId 
                           + "after purchase will have no more in stock");

          }
        else if(Inventory.getInstance().getQuantity(tempOrderlineProductId)
                        > tempOrderlineQuantity)
          {
                    shoppingCart.add(orderline);

Pull this out of the if blocks.

          }
        else if(Inventory.getInstance().getQuantity(tempOrderlineProductId)
                            == -1)

          {
                        throw new OutOfStockException("PrductId: " + tempOrderlineProductId 
                                + " is not listed in the Inventory: "
                                + Inventory.getInstance().getQuantity(tempOrderlineProductId));

           }
         else
          {
                        throw new OutOfStockException("PrductId: " + tempOrderlineProductId 
                                                    + " does not have enough to complete this order, the in Stock amount is: "
                                                    + Inventory.getInstance().getQuantity(tempOrderlineProductId));
          }

Move this entire if block structure into

   Inventory.getInstance().takeQuantity(orderline.getProduct(), orderline.getQuantity());


}

Basically, I'd end up with

public void addToCar(OrderlineType orderline) throws Exception 
{
     orderline.getProduct().verifyIsAllowedToSell();
     Inventory.getInstance().takeQuantity(orderline.getProduct(), orderline.getQuantity());
     shoppingCart.add(orderline);
}

Answer 2

Throwing Exception

The only thing I would add to these answers is to avoid the throws Exception clause in your method definition: this needlessly requires any calling method to have to handle or throw Exception without giving any information about really what kinds of exceptions you are expecting to throw.

Exception-Handling Antipatterns states:

[...] it completely defeats the purpose of using a checked exception. It tells your callers "something can go wrong in my method. [...] Don't do this. Declare the specific checked exceptions that your method can throw.

Checked vs Unchecked

If you have exceptions that truly represent real-world exceptional cases, like the OutOfStockException and ProductRecallException, then make them checked exceptions (don't subclass RuntimeException) and put them in the throws clause. This let's calling methods know that these are actual but recoverable possibilities. Things like NullPointerException, however, are programming errors that should remain unchecked and caught at a higher level.

As Best Practices for Exception Handling states:

If the client can take some alternate action to recover from the exception, make it a checked exception. If the client cannot do anything useful, then make the exception unchecked. By useful, I mean taking steps to recover from the exception and not just logging the exception.

Answer 3

I'll preface this by saying that Java is not my strong point.

Let's start by cleaning up the formatting.

public void addToCart(OrderlineType orderline) throws Exception {
    if (orderline == null) {
        throw new OutOfStockException("Null object pass addToCart");
    }

    if (orderline.getProduct().isRecallStatus()) {
        throw new ProductRecallException("Product Id: " + orderline.getProduct().getProductId() 
                + " is being Recalled");
    }

    int tempOrderlineQuantity = orderline.getQuantity();
    int tempOrderlineProductId = orderline.getProduct().getProductId();

    if (Inventory.getInstance().getQuantity(tempOrderlineProductId) == tempOrderlineQuantity) {
        shoppingCart.add(orderline);
        log.warn("Product " + orderline.getProduct().getProductName()
                +" ID: "+ tempOrderlineProductId 
                + "after purchase will have no more in stock");
    } else if (Inventory.getInstance().getQuantity(tempOrderlineProductId) > tempOrderlineQuantity) {
        shoppingCart.add(orderline);
    } else if (Inventory.getInstance().getQuantity(tempOrderlineProductId) == -1) {
        throw new OutOfStockException("ProductId: " + tempOrderlineProductId 
                + " is not listed in the Inventory: "
                + Inventory.getInstance().getQuantity(tempOrderlineProductId));
    } else {
        throw new OutOfStockException("ProductId: " + tempOrderlineProductId 
                + " does not have enough to complete this order, the in Stock amount is: "
                + Inventory.getInstance().getQuantity(tempOrderlineProductId));
    }
}

Now we're calling the same methods a lot, so let's clean that up.

public void addToCart(OrderlineType orderline) throws Exception {
    if (orderline == null) {
        throw new OutOfStockException("Null object pass addToCart");
    }

    Product product = orderline.getProduct();
    if (product.isRecallStatus()) {
        throw new ProductRecallException("Product Id: " + product.getProductId() 
                + " is being Recalled");
    }

    int orderlineQuantity = orderline.getQuantity();
    int productId = product.getProductId();
    int inventoryQuantity = Inventory.getInstance().getQuantity(productId);

    if (inventoryQuantity == orderlineQuantity) {
        shoppingCart.add(orderline);
        log.warn("Product " + product.getProductName()
                +" ID: "+ productId 
                + "after purchase will have no more in stock");
    } else if (inventoryQuantity > orderlineQuantity) {
        shoppingCart.add(orderline);
    } else if (inventoryQuantity == -1) {
        throw new OutOfStockException("ProductId: " + productId 
                + " is not listed in the Inventory: -1");
    } else {
        throw new OutOfStockException("ProductId: " + productId 
                + " does not have enough to complete this order, the in Stock amount is: "
                + inventoryQuantity);
    }
}

Great, now we can see what we're doing. Let's handle the exceptional cases first.

public void addToCart(OrderlineType orderline) throws Exception {
    if (orderline == null) {
        throw new OutOfStockException("Null object pass addToCart");
    }

    Product product = orderline.getProduct();
    if (product.isRecallStatus()) {
        throw new ProductRecallException("Product Id: " + product.getProductId() 
                + " is being Recalled");
    }

    int orderlineQuantity = orderline.getQuantity();
    int productId = product.getProductId();
    int inventoryQuantity = Inventory.getInstance().getQuantity(productId);

    if (inventoryQuantity == -1) {
        throw new OutOfStockException("ProductId: " + productId 
                + " is not listed in the Inventory: -1");
    }

    if (inventoryQuantity < orderlineQuantity) {
        throw new OutOfStockException("ProductId: " + productId 
                + " does not have enough to complete this order, the in Stock amount is: "
                + inventoryQuantity);
    }

    shoppingCart.add(orderline);

    if (inventoryQuantity == orderlineQuantity) {
        log.warn("Product " + product.getProductName()
                +" ID: "+ productId 
                + "after purchase will have no more in stock");
    }
}

We should be clear about the exceptions that we're throwing.

public void addToCart(OrderlineType orderline) throws OutOfStockException, ProductRecallException {
    if (orderline == null) {
        throw new OutOfStockException("Null object pass addToCart");
    }

    Product product = orderline.getProduct();
    if (product.isRecallStatus()) {
        throw new ProductRecallException("Product Id: " + product.getProductId() 
                + " is being Recalled");
    }

    int orderlineQuantity = orderline.getQuantity();
    int productId = product.getProductId();
    int inventoryQuantity = Inventory.getInstance().getQuantity(productId);

    if (inventoryQuantity == -1) {
        throw new OutOfStockException("ProductId: " + productId 
                + " is not listed in the Inventory: -1");
    }

    if (inventoryQuantity < orderlineQuantity) {
        throw new OutOfStockException("ProductId: " + productId 
                + " does not have enough to complete this order, the in Stock amount is: "
                + inventoryQuantity);
    }

    shoppingCart.add(orderline);

    if (inventoryQuantity == orderlineQuantity) {
        log.warn("Product " + product.getProductName()
                +" ID: "+ productId 
                + "after purchase will have no more in stock");
    }
}

Do we really want to throw an OutOfStockException if orderline is null? I think an IllegalArgumentException would be better.

public void addToCart(OrderlineType orderline) throws IllegalArgumentException, OutOfStockException, ProductRecallException {
    if (orderline == null) {
        throw new IllegalArgumentException("orderline is null");
    }

    Product product = orderline.getProduct();
    if (product.isRecallStatus()) {
        throw new ProductRecallException("Product Id: " + product.getProductId() 
                + " is being Recalled");
    }

    int orderlineQuantity = orderline.getQuantity();
    int productId = product.getProductId();
    int inventoryQuantity = Inventory.getInstance().getQuantity(productId);

    if (inventoryQuantity == -1) {
        throw new OutOfStockException("ProductId: " + productId 
                + " is not listed in the Inventory: -1");
    }

    if (inventoryQuantity < orderlineQuantity) {
        throw new OutOfStockException("ProductId: " + productId 
                + " does not have enough to complete this order, the in Stock amount is: "
                + inventoryQuantity);
    }

    shoppingCart.add(orderline);

    if (inventoryQuantity == orderlineQuantity) {
        log.warn("Product " + product.getProductName()
                +" ID: "+ productId 
                + "after purchase will have no more in stock");
    }
}

It doesn't seem like this method should be responsible for locating the inventory. It is probably better passed as a parameter.

public void addToCart(OrderlineType orderline, Inventory inventory) throws IllegalArgumentException, OutOfStockException, ProductRecallException {
    if (orderline == null) {
        throw new IllegalArgumentException("orderline is null");
    }

    Product product = orderline.getProduct();
    if (product.isRecallStatus()) {
        throw new ProductRecallException("Product Id: " + product.getProductId() 
                + " is being Recalled");
    }

    int orderlineQuantity = orderline.getQuantity();
    int productId = product.getProductId();
    int inventoryQuantity = inventory.getQuantity(productId);

    if (inventoryQuantity == -1) {
        throw new OutOfStockException("ProductId: " + productId 
                + " is not listed in the Inventory: -1");
    }

    if (inventoryQuantity < orderlineQuantity) {
        throw new OutOfStockException("ProductId: " + productId 
                + " does not have enough to complete this order, the in Stock amount is: "
                + inventoryQuantity);
    }

    shoppingCart.add(orderline);

    if (inventoryQuantity == orderlineQuantity) {
        log.warn("Product " + product.getProductName()
                +" ID: "+ productId 
                + "after purchase will have no more in stock");
    }
}

Answer 4

I'm not an expert at programming in any kind of way, and I don't know Java, but I think I can point out a few things that will be useful to you.

Your questions at the end of your post make me believe that you already know that this method is doing too much, and that it can be improved in a number of ways. But on the plus side, you are definitely asking the right questions.

Essentially it looks to me like this method takes an OrderLineType object, validates its state, and executes some code based on its current properties. Your approach works, but the overall logic of the method is obfuscated by the if/else statements. By breaking the method into smaller methods with appropriate names, the readability of the code will be greatly increased.

There are many approaches to accomplishing this, but what I recommend (note, based on my limited experience and knowledge) is to break this method into a validation phase and an execution phase. What happens in the execution phase is based on the results of the validation phase. In the validation phase at the start of the method, execute a method that goes through your if/else logic and decides an outcome. Then have a switch statement that calls another method based on the outcome. In this way the code will be self documenting and easier to understand.

Just an quick, inexact example. First, set a state based on the values of the inventory and orderline:

//Inside the addToCart method or in another method called at the start of that method with the appropriate values passed in
State state = WaitingForEvaluation; //i don't know how to make an enum in Java, sorry :)

public void evaluateState (inventory, orderline) {
    //put the logic in this method to decide the current state based on the inventory and the order line, so:
    if( inventory.getInstance().getQuantity(tempOrderlineProductId) == orderline.getTempOrderlineQuantity) {
        state = InventoryQuantityMatches;
    } else if(Inventory.getInstance().getQuantity(tempOrderlineProductId) > orderline.getTempOrderlineQuantity) {
        state = InventoryQuantityGreater;
    }
}

Now that you have a state, you can use it to decide what logic to run. First you break up the logic of each of the methods into their own methods with appropriate names, and then you use the switch statement to decide which method to call:

//sorry I don't know the syntax for switch statements in Java
switch (state) {
    case InventoryQuantityMatches:
        this.addLastProductToCart(shoppingCart, orderline);
        break;
    }

    case InventoryQuantityGreater:
        shoppingCart.add(orderline);
        break;
}

void addLastProductToCart (shoppingCart, orderline) {
    shoppingCart.add(orderline);
    log.warn("Product " + orderline.getProduct().getProductName()
                        +" ID: "+ tempOrderlineProductId 
                         "after purchase will have no more in stock");
}  

//the rest of the methods that contain the other logic embedded in the former if/else statement

Sorry if the example is unclear, I do not know Java. There must be ways to achieve these basic concepts though. In my opinion this makes the logic of the code much easier to understand. It's more work to set things up this way, but it will be much easier to debug when the time comes. Sorry if you already know how to do all this stuff, as your questions indicate you knew that the code may need to do something like this. I'm a beginner myself so if anyone else has better ideas, please elaborate with an answer.