locked
follow solid principles on new requirement RRS feed

  • Question

  • User1080785583 posted

    Simple e-commerce site. I am using repository pattern. The user now wants to display data that shows a discounted price. With this pattern, do I extend my static repository method?

    public static class ProductRepository
        {
            public static IEnumerable<Product> GetProducts(this IRepositoryAsync<Product> repository)
            {
                return repository
                    .Queryable()
                    .AsEnumerable();
            }
           
           public static GetProductsWithDiscount ???
        }
    

    Then in my data access layer, should I add a new method or a new class based on this interface?

    public interface IProductService
        {
            List<IProduct> GetAvailableProducts();
    
            /// <summary>
            /// new method here
            /// </summary>
            /// <returns></returns>
            List<IProduct> GetDiscountProducts();
        }

    I am using IOC/DI, which means I could derive from my ProductService object since all methods are virtual. But is that how I should approach it? I typically add another method but I don't want to break the code flow by adding a new method with a similar name. My polymorphic instinct tells me same name many forms, derived override, but there might be something more simple...

    I want the logic to tell a product is on discount only in the business layer since items can be turned off and on, etc.

    Thanks in advance.

    Friday, December 5, 2014 7:39 PM

Answers

  • User-1454326058 posted

    Hi,

    Using a service layer is better.

    List<IProduct> GetAvailableProducts();
    
            /// <summary>
            /// new method here
            /// </summary>
            /// <returns></returns>
            List<IProduct> GetDiscountProducts();

    Just use a method with the parameter to identity whether to retrieve available or discount product data.

    Best Regards

    Starain

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Sunday, December 7, 2014 10:15 PM
  • User-1611549905 posted

    The problem that @xequence has outlined in the original post here IMO illustrates one of the biggest failings of the repository pattern as it's commonly implemented. Namely: it doesn't cope well with the kind of changing requirements that you face on a day to day basis in the real world. Adding new requirements (for example, for filtering, sorting, paging etc) often requires you to either add a whole lot of new methods or else new parameters to a method, and if you're not careful, you can easily end up with methods with dozens of parameters. Besides being unwieldy, this is a code smell.

    The alternative is to build a generic Repository which exposes IQueryable directly to your business layer, but here you need to expose all the necessary functionality to allow you to specify prefetch queries, otherwise you're going to end up with select n+1 issues all over the place. This usually means in practice that you have to expose your ORM directly to your business layer, so having an additional Repository Facade on top of it is somewhat redundant and doesn't serve any purpose.

    Have you considered using a Query Object instead? It's a much more flexible approach in terms of changes such as these that actually matter.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Tuesday, December 9, 2014 4:32 AM

All replies

  • User1492915735 posted

    Xequence

    Xequence

    You should make a conscious decision about whether or not you are going to add new method for interface base. If you follow the SOLID. It is Interface Segregation principle or create new Interface.

    If I be you, I would add new method to existing interface rather than create new interface.

    There's two way to achieve this issues

    1. using interface segregation

    2. using virtual but I would refactoring your code like following:

    public interface IProductService
        {
            List<IProduct> Execute();
        }
    
    public class AvailableProducts : IProductService
        {
            public virtual List<IProduct> Execute()
            {}
        }
    
    public class DiscountProducts : AvailableProducts
    {
            protect override List<IProduct> Execute()
            {}
    
    }
    
    There's a few way to achieve without effect the rest code.

    You say that you're using IOC

    Alternative way is create new interface then register new interface by using container.

    public interface IAvailableProductService
        {
            List<IProduct> Execute();
        }
    
    public interface IDiscountProductService
        {
            List<IProduct> Execute();
        }
    
    public class AvailableProductService :  IAvailableProductService
        {
            List<IProduct> Execute();
        }
    
    public class DiscountProductService : IDiscountProductService
        {
            List<IProduct> Execute();
        }
    
    Seems odd but it works

    Friday, December 5, 2014 8:25 PM
  • User-1454326058 posted

    Hi,

    Using a service layer is better.

    List<IProduct> GetAvailableProducts();
    
            /// <summary>
            /// new method here
            /// </summary>
            /// <returns></returns>
            List<IProduct> GetDiscountProducts();

    Just use a method with the parameter to identity whether to retrieve available or discount product data.

    Best Regards

    Starain

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Sunday, December 7, 2014 10:15 PM
  • User1080785583 posted

    I did both but ended up extending my IProduct into IDiscountProduct and returning a list. Only because I needed both types in my return set, discount and non-discount, do I return IDiscountProduct.

     

    List<IDiscountProduct> GetMasterProductList();

    This way in my ProductService I can inject my DiscountService ctor, then add the  logic without having to rebuild any other methods.

    Granted my methods are virtual, I would still like to be able to reuse the base implementation at this point.

    public interface IDiscountProduct : IProduct
        {
            double DiscountedCost { get; set; }
            string ComputedDiscountDisplay { get;  }
        }

    Monday, December 8, 2014 10:55 AM
  • User1492915735 posted

    Either ways of the above is acceptable and seems like there's no side effect.

    Following might help

    Take a look at creation section

    http://www.dofactory.com/net/design-patterns

    Monday, December 8, 2014 2:34 PM
  • User-1611549905 posted

    The problem that @xequence has outlined in the original post here IMO illustrates one of the biggest failings of the repository pattern as it's commonly implemented. Namely: it doesn't cope well with the kind of changing requirements that you face on a day to day basis in the real world. Adding new requirements (for example, for filtering, sorting, paging etc) often requires you to either add a whole lot of new methods or else new parameters to a method, and if you're not careful, you can easily end up with methods with dozens of parameters. Besides being unwieldy, this is a code smell.

    The alternative is to build a generic Repository which exposes IQueryable directly to your business layer, but here you need to expose all the necessary functionality to allow you to specify prefetch queries, otherwise you're going to end up with select n+1 issues all over the place. This usually means in practice that you have to expose your ORM directly to your business layer, so having an additional Repository Facade on top of it is somewhat redundant and doesn't serve any purpose.

    Have you considered using a Query Object instead? It's a much more flexible approach in terms of changes such as these that actually matter.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Tuesday, December 9, 2014 4:32 AM
  • User1080785583 posted

    jammycakes, in short you are correct. my problem is exactly the changing requirements. with ioc, one could argue i simply add a new class and reimplement, successfully moving beyond abstract factory pattern at runtime. The concept I think that applies here is open to extension, closed to modification. Albeit I will probably most likely possibly never modify an unused method..

    Yet, I must be entirely virtual to attempt such a virtue. My methods are able to be overwritten but then again, simplicity has 20/20 hindsight.

    I don't exactly use the 'repository' pattern in its entirety. It has been abstracted to the point that my server side architecture does not affect my ui. I have services that have runtime dependencies on other services and repositories. Here is a small example that exposes the need to maintain IQueryable. My business layer will be injected the same way a _discountService will be injected, allowing me reuse of common requirements.

     

    public interface IProductService
        {
            List<IProduct> GetAvailableProducts(); 
    
            /// <summary>
            /// new method here
            /// </summary>
            /// <returns></returns>
            List<IDiscountProduct> GetMasterProductList();
        }
    
    public static class ProductRepository
        {
            public static IEnumerable<Product> GetProducts(this IRepositoryAsync<Product> repository)
            {
                return repository
                    .Queryable()
                    .AsEnumerable();
            }
    
            /// <summary>
            /// do not enumerate
            /// </summary>
            /// <param name="repository"></param>
            /// <returns></returns>
            public static IQueryable<Product> GetProductsQueryable(this IRepositoryAsync<Product> repository)
            {
                return repository
                    .Queryable();
            }
        }
    
    public class ProductService :  IProductService
        {
            private readonly IRepositoryAsync<Product> _productRepository; 
            private readonly IDiscountService _discountService;
    
            public ProductService(IRepositoryAsync<Product> productRepository, IRepositoryAsync<Inventory> repositoryInventory, IRepositoryAsync<Zone> repositoryZone, IDiscountService discountService)
            {
                _productRepository = productRepository;
                _repositoryInventory = repositoryInventory;
                _repositoryZone = repositoryZone;
                _discountService = discountService;
            }
    
            public virtual List<IProduct> GetAvailableProducts()
            {
                try
                {
                    var items = (from r in _productRepository.GetProducts()
                                 select r).ToList();
    
                    var returnList =
                        new List<IProduct>((from r in items.Select(u => new ProductModel(u.Id, u.ImageUrl, u.Name, u.Cost, u.Description))
                                            select r).ToList());
    
    
                    if (returnList.Any())
                    {
                        return returnList;
                    }
                }
                catch
                {
                    throw;
                }
    
                // error
                return null;
            }        
    
            
    
            /// <summary>
            /// this method is used to display the computed discount product
            /// for the list of products
            /// </summary>
            /// <returns></returns>
            public virtual List<IDiscountProduct> GetMasterProductList()
            { 
                // could get expensive with GetAvailableProducts, should be IQueryable as long as possible
                var discountedItems = (_discountService.DiscountProducts(this.GetAvailableProducts()));
    
                // could get expensive with GetAvailableProducts, should be IQueryable as long as possible
                var initNonDiscountList = (_discountService.NonDiscountProducts(this.GetAvailableProducts()));
    
                // iterate each non discount item and turn it into a discount item with 0 discount value
                foreach (var nonDiscountItem in initNonDiscountList)
                {
                    discountedItems.Add(new DiscountProduct(nonDiscountItem.ProductId,nonDiscountItem.ImageUrl,
                        nonDiscountItem.ProductName,nonDiscountItem.Cost,nonDiscountItem.Description, 0));
                }
    
    
                return discountedItems;
    
            }
    
    
             
           
        }
    }

    Discount service, in short, that allows my business logic to help me.

    public class DiscountService : IDiscountService
        {
            private readonly IDiscountDomainLogic _discountDomainLogic;
    
            public DiscountService(IDiscountDomainLogic discountDomainLogic)
            {
                _discountDomainLogic = discountDomainLogic;
            }
    
            /// <summary>
            /// new discount method to handle any product
            /// </summary>
            /// <param name="cartItems"></param>
            /// <returns></returns>
            public virtual List<IDiscountProduct> DiscountProducts(List<IProduct> cartItems)
            {
                var discountProducts = new List<IDiscountProduct>();
                foreach (var product in cartItems)
                { 
                    var enumProduct = (EnumProducts)product.ProductId;
    
                    if (_discountDomainLogic.ValidDiscount(enumProduct))
                    {
                        var discountProduct = _discountDomainLogic.DiscountProduct(product);
                        discountProducts.Add(discountProduct);
                    } 
                }
                return discountProducts;
            } 
    
            /// <summary>
            /// method should be refactored to use discount domain logic
            /// to determine how to calculate
            /// </summary>
            /// <param name="cartItems"></param>
            /// <returns></returns>
            public virtual double CalculateCostNoDiscount(List<IProduct> cartItems)
            {
                var products = new List<IProduct>();
                foreach (var product in cartItems)
                { 
                    var noDiscountProduct = _discountDomainLogic.NonDiscountProduct(product);
    
                    if (noDiscountProduct != null)
                    {
                        products.Add(product);
                    } 
    
                }
                return products.Sum(f => f.Cost);
            } 
        }

    Am I truly using Interface Segregation? Or am I really using open to extension closed to modification? Where is Liskov appearing in this code? How can I make it not smell?

    Don't get me wrong, I am a HUGE PROPONENT of Expression modification of the LINQ Trees at runtime and caching the nodes, but this project does not need that yet. I am not querying thousands of products.

    Tuesday, December 9, 2014 1:14 PM