3
\$\begingroup\$

I am writing a code to search Product database and return result to user. user can select various options like product attributes that varies from product to product, also there may be case like for single attribute there can be multiple value. For Example user can select brand as filter and can select from given brand under brand option. There can be multiple tag for filter like this

Please guide me to improve this code, is there any design pattern i should use or can replace some statement with other to improve in any Sense.

public Models.Custom_Model.SubcategoryProductViewModel SubCategoryAds(long subcategoryId, Models.Custom_Model.SearchModel search)
    {
        var products = _db.products.Include(a => a.category).Include(c => c.subcategory).Include(c=>c.productattributes).Where(c => c.SubCategoryId == subcategoryId && c.subcategory.isActive && c.category.isActive);
        switch (search.criteria)
        {
            case Enum.SortingCriteria.New:
                products = products.OrderByDescending(c => c.ProductId);
                break;
            case Enum.SortingCriteria.Discount:
                products = products.OrderByDescending(c => c.Discount);
                break;
            case Enum.SortingCriteria.PricebyAsc:
                products = products.OrderBy(c => c.Price);
                break;
            case Enum.SortingCriteria.PricebyDsc:
                products = products.OrderByDescending(c => c.Price);
                break;
        }
        if(search.filter != null)
        {
            var fs = search.filter.SelectMany(c => c.Value.Select(a => new Models.Custom_Model.IdName {Id = c.Id,Name = a}));
            products.Where(z=>((z.productattributes.Select(c=>new Models.Custom_Model.IdName {Id = c.AttributeId,Name = c.Value})).Intersect(fs)).Any());
        }
        var productids = products.Skip((search.pageNo - 1) * search.pagesize).Take(search.pagesize).Select(c => c.ProductId).ToList();
        var subcategory = _db.subcategories.Include(c => c.category).SingleOrDefault(c => c.SubCategoryId == subcategoryId);
        string Name = string.Empty;
        Models.Custom_Model.IdName category = null;
        if (subcategory != null)
        {
            Name = subcategory.Name;
            category = new Models.Custom_Model.IdName() { Id = subcategory.CategoryId, Name = subcategory.category.Name };
        }
        var count = products.Count();
        var product = Business_Logic.ProductTile.ToproductTile(productids, _db);
        return new  Models.Custom_Model.SubcategoryProductViewModel {Category = category ,Name = Name,TotalItems = count, Products = product };
    }

search Model

public class SearchModel
{
    private int _pagesize = 50;

    public int pagesize
    {
        get { return _pagesize; }
        set { _pagesize =value; }
    }

    private int _pageNo = 1;

    public int pageNo
    {
        get { return _pageNo; }
        set { _pageNo = value; }
    }

    private Enum.SortingCriteria _criteria = Enum.SortingCriteria.New;

    public Enum.SortingCriteria criteria
    {
        get { return _criteria; }
        set { _criteria = value; }
    }

    private List<FiltersViewModel> _filter = null;

    public List<FiltersViewModel> filter
    {
        get { return _filter ; }
        set { _filter  = value; }
    }


}

FilterViewModel

public class FiltersViewModel
{
    public long Id { get; set; }

    public IEnumerable<string> Value { get; set; }
}
\$\endgroup\$
  • \$\begingroup\$ If you place the context of your question above the code you will attract more attention because how the question looks on the frontpage. \$\endgroup\$ – Heslacher Feb 12 '17 at 6:59
0
\$\begingroup\$

At least a couple things to note:

Bug?

I think this part doesn't work because you don't assign the result of this query back to the variable.

if(search.filter != null)
{
    var fs = search.filter.SelectMany(c => c.Value.Select(a => new Models.Custom_Model.IdName {Id = c.Id,Name = a}));
    products.Where(z=>((z.productattributes.Select(c=>new Models.Custom_Model.IdName {Id = c.AttributeId,Name = c.Value})).Intersect(fs)).Any());
}

Naming

Public properties should be in PascalCasing (msdn) private variables in camelCase.

Also, some using-statements would remove the namespace-clutter.

Line breaks

I think couple of line breaks would make long lines more readable. First line as an example.

var products = _db.products
    .Include(a => a.category)
    .Include(c => c.subcategory)
    .Include(c => c.productattributes)
    .Where(c => c.SubCategoryId == subcategoryId && c.subcategory.isActive && c.category.isActive);

Refactoring to smaller methods

Pagination (skipping and taking x records) is pretty general concept, so it should perhaps be moved to to its own, generic function.

Also, sorting could be moved to its own method

SearchModel refactoring

I remove private variables from SearchModel because they are only used for defaults. I would set defaults in constructor so defaults are in one place. Filter doesn't need to be set but I set it here for consistency.

I also renamed Criteria to SortingCriteria to make the meaning clearir. If it is only Criteria, it is not clear if it is used to filter the query or sorting.

public int PageSize { get; set; }
public int PageNumber { get; set; }
public Enum.SortingCriteria SortingCriteria { get; set; }
public List<FiltersViewModel> Filter { get; set; }

public SearchModel()
{
    PageSize = 50;
    PageNumber = 1;
    SortingCriteria = Enum.SortingCriteria.New;
    Filter = null;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.