Search code examples
c#.netdesign-patternssolid-principles

Moving data between interfaces without violating SOLID principles?


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.


Solution

  • 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.