Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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));
          }
}
share|improve this question

4 Answers 4

up vote 4 down vote accepted
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);
}
share|improve this answer
2  
I disagree with "Avoid storing variables in temporary locals unless you really need to" since it can improve the readability. That said, in this case it's obscuring a design flaw, but instead of typing a 60-character method chain I'd rather store it in a temporary variable to increase readability. –  Pimgd Jul 31 at 7:03
    
@Pimgd, a 60 character method chain is suspect, but I agree there certainly are cases where a temporary local is helpful. I treat them with suspicion because they are often hiding design flaws. –  Winston Ewert Jul 31 at 14:23

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.

share|improve this answer
    
Is there a part of the link you could add to your answer as small quote or your first paragraph already cover that ? –  Marc-Andre Jul 31 at 15:42
1  
I think a quote or two would help a lot - thanks. The link talks more about when to use checked vs. unchecked exceptions but has a little blurb about using Exception as well. I'll take a look. –  Jake Toronto Aug 1 at 14:37

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");
    }
}
share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.