HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Add-to-cart method has too many conditions

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
conditionsmethodtoohascartmanyadd

Problem

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

Solution

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);
}

Code Snippets

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");
        }
orderline.getProduct().verifyAllowedToSell();
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);

Context

StackExchange Code Review Q#58582, answer score: 4

Revisions (0)

No revisions yet.