Breaking up a class with too many responsibilities

Question

I'm looking for advice on my specific problem, how to break up a class, Basket, that has too many responsibilities.

Currently, it does three things:

• Keeps track of the products in the basket: addProduct(), removeProduct(), getProductQuantity()
• Keeps track of the total price of the basket: getTotalBill()
• Applies discounts: applyDiscount()

Products can have multiple distinct discounts applied to them, but not the same discount more than once. In order to facilitate this here's my design:

I can see the following problems with this design:

• Circular dependency between Discount (that needs a basket in applyDiscount) and Basket (that stores a map of Discount codes)
• Basket has too many responsibilities. It keeps track of products in the basket, the price and discounts
  • A real life basket only keeps hold of items, not the price or discounts

I'm struggling to think of ways to improve this design. I've thought of an alternative idea but it has its tradeoffs:

• Have a BillCalculator that calculates the total.
  • Users want to know their current cart total at all times, so we'd have to call this each time we add or remove a product from Basket and recalculate the bill from scratch. This is inefficient compared to the current approach where totalQuantity is simply incremented and decremented when addProduct or removeProduct is called.

What am I missing? How can I break up the multiple responsibilities of Basket without sacrificing on efficiency, and without storing the list of products in multiple places?

Answer 1

Applying SRP the right way

According to its inventor Uncle Bob, the Single Responsibility Principe is not about what a class does, but about its reason to change. In this regard:

• A basket, which allows to add and remove products and check out at the end, appears to be something relatively stable that may change in only in relation with the way the buying experience is organized.
• Determining prices and discounts appears to be something that changes much more frequently, depending on the creativity of the marketing people. It start with products discounts, then it's discount for combination of products, then it may be fidelity discounts for frequent buyers, and who knows where this will stop.

Price determination is always a tricky topic not only because of the creative discounts, but also in relation to taxes, country specific rules etc. For this reason the basket may maintain in the end the price, but the calculations should really be isolated.

How to decouple discounts from the basket?

A good way would be to use the visitor pattern to visit the basket and its product to determine if the condition apply and to calculate the discount. Different kind of discount may be implemented by specializing the discount (or implementing its interface):

However, the discount should not by itself change the total of the bill. Because, you state that only one discount can be applied per product. Therefore, if several discount could be applicable, it must be decided which discount to apply before updating the total: the best one for the customer? the best one for the vendor? the first one? all the applciable ones? To address this issue, you could use a strategy pattern to verify the applicable discounts, and chose or combine them based on some underlying rule. If the rules change, you may just need to specialize the strategy.

This split could look like:

] If the split of responsibility that the combination of these two patterns inspire you and suit the needs, your question is almost answer. You'd still have to decide who shall invoke the strategy in the end: should it be a bill calculator? or the basket itself? Some more thoughts would be needed here. It depends inter alia if you want to update the price real time as customer adds products, or if you have a checkout process with some other classes involved. But I think that this is fine tuning.

Important remark: An important difference here is also the fact that the basket is no longer a star that has to know all the details and how they are related. This allows to delegate some operations at the level of the item

Answer 2

Most likely you are missing what "single responsibility" means.

The chef in my favourite restaurant makes steaks, sausages, pork fillet, fish, mussles, prawns, six different kinds of potatoes, 12 different kinds of vegetables, eight puddings and a mean cheese board.

He has a single responsibility: To cook.

And that's probably why you can't figure out how to split up your class. Just like you can't split up the chef's responsibilities into 30 different foods. Just write down what the Basket's single responsibility is.

Yes, you don't need both the Discount to know about the Basket and the Basket to know about the Discount. Most likely the Basket should know about triplets (Product, Quantity, Discount) which are encapsulated in your CartItem.

Answer 3

BillCalculator is on the right path. You need to separate out the items from the prices. This is counter intuitive from a users point of view, but makes sense from a traditional business accounting perspective.

You have orders and invoices for those orders. The invoice has the prices on and they are only loosely dependent on the items in the order. You can have discounts, freebies, bulk discounts, taxes, customer dependent prices, date dependent prices etc etc.

I would have some structs/POCOS

Order
{
    Items
}

Invoice
{
    InvoiceLines
    Total
}

InvoiceLine
{
   Description
   Amount
   Tax
}

And then Objects to work on them

InvoiceGenerator
{
    GenerateInvoice(Order o)
}

Basket  //ui helper object
{
    private invoiceGenerator
    Order
    ExpectedInvoice
    AddItem()  //work out new expected invoice
    RemoveItem() //work out new expected invoice
}