What is SRP?
I want to emphasize that SRP, S from SOLID is Single Responsibility Principle. Means that a class / object only has 1 responsibility and no more. If the class is changed, then there is for 1 and only 1 reason, no more.
IEnumerable?
Second, why do you use IEnumerable
(instead of List
) and using Concat
? I can't find the pros of using IEnumerable
and Concat
over List
.
Now let's look at the implementation of your CartHelper
.
First from the naming, it is Helper
. What is it / what is the helper? What is the responsibility?
AddItem
Moving to AddItem
, I can see that the logic is:
- using the product id provided, get the product using service.
- If the product exists and product type is
Subscription
, add into Subscription
.
- Last, add into
Product
list and CartService
.
In my point of view, the method is doing too much. I think it will better if you shift point 1 somewhere else, or even the client will be ok. It will make the AddItem to only accept IProductBase
and add into Product
list. What about Subscription
list? Scrap that, you don't need it. Using LINQ where
, you can find subset of subscriptions from Product
list using its type property.
From client point of view, what can this method provide to user when a specified product is not found during AddItem
? You don't throw
any exceptions or anything. Client will just add the product
and the they will not know whether the product has been added or not. They will just expect that everything has been done and no error handling there.
I don't understand what is CartService
doing. However I think it will be fine if you want to implement like that.
RemoveItem
Same as AddItem
, you can scrap the Subscription
list. Regarding passing the product id or IProductBase
class, it is comparable each other.
GetCartTotalBeforeDiscount
Wait, this class can add item, remove item, and now get the total? Following SRP, if you want to change the logic for getting total before discount, you need to modify this class. Now it has 2 reasons for changing the class, 1st is if you want to change how it manage item collection (add/remove/get) and 2nd how to calculate the total before discount. Shift this responsibility into other class. If you think it is convertible to add the method in CartHelper
, use a facade pattern.
GetCartItems
Now you want to get the cart items. But wait, you converted it into a specific class instead of just returning it? What is the use of IProductBase
interface when you just converting each product into specific class? Moreover, now it has 3 reasons for changing the class, now with conversion as a reason.
So how do you want to improve this GetCartItems? Change into GetProducts
and return the IEnumerable
of IProductBase
and everything is done in the class. Then create some interfaces
for product conversion such as:
class CartItemConverter : IProductConverter<ICartItem>{
public IEnumerable<ICartItem> GetConverted(IEnumerable<IProductBase> products){
// logic here
}
}
Now you has the freedom for conversion.
Note: There maybe other better pattern such as Decorator
for this case. but IMHO this one is the simplest one and having the most similar structure with your code.