Moving data between interfaces without violating SOLID principles?

Question

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 IStoreProducts and IStoreSubscriptions 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 IProducts)? 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.

Answer 1

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.

Answer 2

My simple answer is that a StoreProduct is not a cart item. You could have a ProductCartItem which implements ICartItem. This implementation would contain a reference to IStoreProduct. Your cart item has things a product doesn't, like quantity, discount price, current price, etc.

In this way you're not violating SRP or other SOLID principles. Your cart item might record the price at the time the product was added to the cart, even if the price for in shelf items changes. The cart item could also use the StoreProduct for its upc, without you needing to store it in the cart item itself.

I think this is the direction you should head in as the design will allow you to add cart items which are not products (say ServiceCartItem).

Finally, you may need some other types, perhaps a ProductCartItem factory that constructs a new cart item from the product. It depends on how much logic is involved in adding something to a cart.

Answer 3

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:

1. using the product id provided, get the product using service.
2. If the product exists and product type is Subscription, add into Subscription.
3. 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.