8

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:

Class Diagram

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?

5
  • 1
    So, if you've already determined that there are, in your view, 3 different responsibilities, why don't you separate things so that each is covered by a self contained class (or s small group of cooperating objects)? R1: Keeping track of the products in the basket - create a BasketItemList (a custom collection). R2: Applying discount rules - create something that encapsulates these rules (could take the BasketItemList as a parameter). R3: Keeping track of the total price - coordinate the previous two together in an abstract way (forwarding the details to code handling R1 & R2). Dec 10, 2020 at 16:01
  • These classes would be very tightly coupled and would all need to be aware of the internal state of the other. R1 and R3 would need a way to know which product corresponds to which item on the bill so it can be removed when the item is removed from the basket. Dec 10, 2020 at 17:19
  • "These classes would be very tightly coupled" - that entirely depends on how you design the interface between them (assuming you've got your responsibilities right; separating by responsibilities should result in loose coupling). Also, you've sad "I've thought of an alternative idea but it has its tradeoffs" - every idea you think of is going to have tradeoffs; design is about which tradeoffs are right for your system. Dec 10, 2020 at 19:07
  • Your class diagram tells that the Basket inherits from CartItem, Product and Discount. I don't think that this is really what you mean, so could you remove the white triangles at the end of the association, and either put nothing, or an open arrow (if you want to document navigability) ?
    – Christophe
    Dec 11, 2020 at 0:42
  • Just a little question. How inefficient is the BillCalculator? If it's not really affecting the user experience that much, maybe just go for it. Premature optimization is the root of all evils, as they say. Dec 12, 2020 at 1:18

3 Answers 3

7

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:

enter image description here] 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

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.

4
  • Thanks for the suggestion. But that wouldn't work, with the triples you're assuming that each discount applies to only one product. However many discounts (such as a meal deal with a sandwich, snack and drink) apply to many products. If the snack is removed from the basket, we also have to remove the discount from the drink and sandwich, so a triplet mapping would not work in this case. Dec 10, 2020 at 13:15
  • Adam, so what? You have (ham sandwich, 5, 10% discount pensioner discount), (jam sandwich, 3, 5% kids discount) and (cheese sandwich, 6, 10% pensioner discount). Why would removing the ham sandwich from the basket remove the other two discounts? That's nonsense.
    – gnasher729
    Dec 10, 2020 at 15:56
  • 2
    No, you're not understanding. It's a Meal Deal. If you have a sandwich, a snack and a drink in your basket, you get a 25% discount on all of the items. All Items must be present, if you remove the drink from the basket, it's no longer a meal deal and no items will be discounted. Dec 10, 2020 at 16:24
  • +1 for the explanation of single responsibility. Nevertheless, I think that the cook does generally calculate the invoice and some more thoughts could be useful to get a more general pricing mecansim (that could cope also with other kinds of basket related calculation scheme).
    – Christophe
    Dec 11, 2020 at 0:40
1

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
}
5
  • Thanks for the pointer. It seems like it's on the right lines, but what exactly is Line? We need it so we can generate an itemised bill of course, but it seems like it's essentially storing all the same information as Order. Both Order and Lines must contain the same information: the name of the product and the quantity of the product. What advantage do we get from storing this information in two separate places? Dec 10, 2020 at 16:29
  • @AdamGriffiths an order is what is to be done. An invoice is a record of what was done. The prices can change and affect an order. Items can be added and removed from an order. Quantity of items can change in an order. But you can't change an invoice. An invoice is a record of what happened. It's not two copies of the same thing. One behaves very differently than the other. Dec 10, 2020 at 16:47
  • Isn’t this POCOs approach implementing some kind of anemic domain model?
    – Christophe
    Dec 11, 2020 at 7:38
  • yes, the best approach
    – Ewan
    Dec 11, 2020 at 10:25
  • Invoice lines might relate to an order item, but they can also be a discount, tax, delivery charge etc etc
    – Ewan
    Dec 11, 2020 at 10:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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