2

I am working on an e-commerce website using ASP.NET MVC 5, EF6 and LINQ. I have a Products table in my database. In my UI, I have multiple parameters for the filtering of my products:

  • checkboxes for different categories
  • minimum and maximum prices
  • checkboxes for different colors

And I have written this action method:

[HttpPost]
public PartialViewResult FilterProducts(int[] categoriesIds, decimal minPrice, decimal? maxPrice, string[] colors)
{
        if (categoriesIds == null)
        {
            var randomProducts = db.Products
                .OrderBy(p => Guid.NewGuid());
            return PartialView("_LoadProducts", randomProducts.ToList());
        }
        else
        {
            var filteredProducts = db.Products
                .Where(p => categoriesIds.Contains(p.CategoryId)
                    && (p.DiscountedPrice >= minPrice
                        && p.DiscountedPrice <= maxPrice || maxPrice == null));

            if (colors != null)
            {
                filteredProducts = filteredProducts
                    .Where(p => colors.Contains(p.Color));
            }
            return PartialView("_LoadProducts", filteredProducts.ToList());
        }
}

This works fine. But I'm confused to whether this can be improved? It includes a lot of if/else statements just to filter products. Is there any problem with my design overall? Are there any best practices? Any help will be highly appreciated. Thanks.

Aneeq
  • 416
  • 3
  • 15
  • An alternative would be to use a [faceted search framework](http://stackoverflow.com/a/34303688/181087) to do this instead of LINQ. The advantage is that you don't have to deal with the AND/OR logic and can instead use `SimpleFacet` or `RangeFacet` and they will automatically return all results if no filters are selected. – NightOwl888 Jul 08 '16 at 23:50

2 Answers2

3

You could extract your business logic into a helper or a service class (e.g. ProductsService) and your database queries per entity to a repository layer (e.g. ProductsRepository, UsersRepository, etc...)

public class ProductsRepository : BaseRepository<Product> // You can have base implementation for basic CRUD operations
{
     MyDbContext db;
     public ProductsRepository(MyDbContext db)
     {
         this.db = db;
     }

     public IQueryable<Product> GetAll()
     {
          return db.Products;
     }
}

public class ProductsService : BaseService<Product> // again here....
{
     ProductsRepository repo;

     public ProductsService()
     {
          repo = new ProductsRepository(new MyDbContext()); // Intentionally not using Dependency Injection here for simplicity....
     }

     public List<Product> FilterBy(int[] categoriesIds, decimal minPrice, decimal? maxPrice, string[] colors){
     {
         if (categoriesIds == null)
         {
             var randomProducts = repo.GetAll().OrderBy(p => Guid.NewGuid());
             return randomProducts.ToList();
         }
         else
         {
          var filteredProducts = repo.GetAll()
            .Where(p => categoriesIds.Contains(p.CategoryId)
                && (p.DiscountedPrice >= minPrice
                    && p.DiscountedPrice <= maxPrice || maxPrice == null));

        if (colors != null)
        {
            filteredProducts = filteredProducts
                .Where(p => colors.Contains(p.Color));
        }
        return filteredProducts.ToList();
    }
}

MVC

ProductsService productsService;
public MyController() // constructor
{
   productsService = new ProductsService(); // Again, no Dependency Injection here for simplicity
}

[HttpPost]
public PartialViewResult FilterProducts(int[] categoriesIds, decimal minPrice, decimal? maxPrice, string[] colors)
{
     List<Product> products = productsService.FilterBy(categoriesIds, minPrice, maxPrice, colors);

     return PartialView("_LoadProducts", products);
}

So what's the point here... as you can see, you have more code than usual but this approach makes your code better separated and easily reusable. You can use the FilterBy method in ProductsService anywhere in your application over and over again, without having to repeat your queries multiple times. Also your controller is much lighter and easily readable, which is the best approach - you shouldn't have heavy business logic/database operations directly in your controller. That's why it's better to separate your database queries and your business logic into separate files and reuse them where possible. This leads to much less bugs. This is the first rule of SOLID principles - a class or method should have only one responsibility - that means if your method is going to return items from the database, it should do ONLY that and nothing more.

Hope I was useful!

George Findulov
  • 769
  • 6
  • 16
  • I like your answer, and yeah I'm going to use the **Repository Pattern** but still I need to refine my query whether I place it in a repository or a controller. – Aneeq Jul 09 '16 at 00:02
  • Avoid placing your database queries in the controller, that's why the repositories are for. If you have business logic regarding querying one or more entities, use service layer for that purpose. Finally in your controller action you will consume the service. Please, if my post has helped you, mark it as an answer :) – George Findulov Jul 09 '16 at 08:06
  • I actually wanted to simplify the query. But still your answer has helped in organizing my project. Thanks. – Aneeq Jul 11 '16 at 16:34
2

Adding to the answer of @GeorgeFindulov

Make your input as a standard predefined object

public class FilterDto
{
    public int[] Categories { get; set; }
    public int? MinPrice{ get; set; }
    public int? MaxPrice{ get; set; }
    //...
}

This makes your controller clean and unchanged for new inputs. if you have some new filter in future you will simply add a new property to the class FilterDto keeping the action method as is. You can just convert this DTO to a business object using auto-mapper.

[HttpGet]
public PartialViewResult FilterProducts(FilterDto filters)
{
    FilterModel filterModel = Mapper.Map<FilterModel>(filters);
    List<Product> products = productsService.FilterBy(filterModel);
    return PartialView("_LoadProducts", products);
}

Note: Your service should always return a business model than a specific model and then your controller converts to DTO or a view model.

aditya
  • 575
  • 7
  • 19