TL;DR: What's the best way to move data between interfaces without violating SOLID principles?
I might be over-thinking this and I don't intend to be dogmatic in regard to SOLID principles; but I wanted to get some input. I've been refactoring a shopping cart to be more "SOLID" and there's a method that I wrote that seems like "code smell" to me (maybe it isn't).
I have a CartHelper
class that looks something like this (I've simplified it a bit for brevity):
public class CartHelper { IEnumerable Products; IEnumerable Subscriptions; // ...Other Class Methods... [HttpPost] public void AddItem(int productVariantID) { var product = ProductService.GetByVariantID(productVariantID); if (product != null) { if (product.Type == (int)Constants.ProductTypes.Subscription) Subscriptions = Subscriptions.Concat(new [] { product }); Products = Products.Concat(new [] { product }); CartService.AddItem(productVariantID); } } [HttpPost] public void RemoveItem(int productVariantID) { Subscriptions = Subscriptions.Where(s => s.VariantID != productVariantID); Products = Products.Where(p => p.VariantID != productVariantID); CartService.RemoveItem(productVariantID); } public decimal GetCartTotalBeforeDiscount() { return Products.Sum(p => p.Price); } public IEnumerable GetCartItems() { var products = (from p in Products select new CartSummaryItem { ProductID = p.ProductID, Title = p.Title, Description = p.Description, Price = p.Price, // ...Assign other applicable properties here... } as ICartSummaryItem); return products; } // ...Other Class Methods... }
The part that seems like "code-smell" to me (and there might be more that's bad here) is the GetCartItems()
method. Something about it seems funky to me, but I can't think of an alternative that is any better.
I'm converting to ICartItem
because there are some properties added that need to be passed to the view but they don't make sense on an IStoreProduct
or an IStoreSubscription
(Interface Segregation Principle).
I've thought about adding ConvertProductToCartItem()
and ConvertSubscriptionToCartItem()
methods to the CartHelper
, but that seems like a violation of Single Responsibility Principle. Would it make sense to have a CartItemFactory that accepts IStoreProduct
s and IStoreSubscription
s and for conversion? That seems like a lot of unnecessary overhead for such a simple conversion.
One of the solutions that I came up with is to define an explicit cast method:
public class StoreProduct : IStoreProduct { public decimal Price { get; set; } public decimal Discount { get; set; } // ...Properties... public ICartItem ToCartItem() { // Will call explicit cast implementation return (CartItem) this; } // Explicit cast conversion for Product as CartItem public static explicit operator CartItem(StoreProduct product) { return new CartItem() { Price = product.Price, Discount = product.Price, SalePrice = Helper.CalculateSalePriceForProduct(product), // ...Assign other applicable properties here... }; } }
Which let's me change my GetCartItems
method to this much cleaner implementation:
public IEnumerable GetCartItems() { return Products.Select(p => p.ToCartSummaryItem()); }
But the problem with this approach is that this also violates Single Responsibility Principle by coupling ICartItem
and CartItem
to the StoreProduct
Class. I also considered an extension method instead of the cast conversion, but that's not any better or different.
Should I just make my concrete StoreProduct
class implement the ICartItem
interface and put the cart-specific properties there? Maybe I should just rewrite the CartHelper
so that it only has ICartItems (i.e., remove the IProduct
s)? Both of those options seem like violations of the Single Responsibility Principle. Maybe the solution will become obvious after I get some sleep...
So, I guess what my question boils down to is: what's the best way to move data between interfaces without violating SOLID principles?
Any suggestions? Maybe I should just move on and not worry about it (i.e., don't be dogmatic about SOLID)? Have I answered my own question? Maybe this belongs in programmers.stackexchange, I hope this isn't too subjective.
Also, in case it's helpful, this is what my interfaces look like:
public interface IProductBase { int ProductID { get; set; } decimal Price { get; set; } string Title { get; set; } string Description { get; set; } // ... Other Properties... } public interface IStoreProduct : IProductBase { int VariantID { get; set; } decimal Discount { get; set; } // ... Other Properties... ICartItem ToCartItem(); } public interface ISubscription : IProductBase { SubscriptionType SubscriptionType { get; set; } // ... Other Properties... ICartItem ToCartItem(); } public interface ICartItem : IProductBase { decimal SalePrice { get; set; } // ... Other Properties... }
Update: Added post attributes for clarity.
Ok, so thanks to your suggestions (Fendy, Andy, MrDosu), here's the far-cleaner Facade implementation:
public class Cart
{
private List<ICartItem> CartItems;
// ...Other Class Methods...
[HttpPost]
public void AddItem(int productVariantID)
{
var product = ProductService.GetByVariantID(productVariantID);
if (product != null)
{
CartItems.Add(CartItemConverter.Convert(product));
CartService.AddItem(productVariantID);
}
}
[HttpPost]
public void RemoveItem(int productVariantID)
{
CartItems.RemoveAll(c => c.VariantID == productVariantID);
CartService.RemoveItem(productVariantID);
}
public IEnumerable<ICartItem> GetCartItems()
{
return CartItems;
}
// ...Other Class Methods...
}
Since I only care about cart-related properties after my items have been added to the cart, I can remove references to IStoreProduct and ISubscription. If I ever end up needing to extract more information (YAGNI applies here), I can use the Adapter pattern (via the ProductService) to populate the necessary data.