patternjavaMinor
Add-to-cart method has too many conditions
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
```
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.