9

I have written some code to allow filtering of products on our website, and I am getting a pretty bad code smell. The user can select 1-* of these filters which means I need to be specific with the WHERE clause.

I think I am looking for a way to build up a lambda expression, so for every filter I can 'modify' my WHERE clause - but I am not sure how to do this in .NET, and there must be a way.

Code in its current state (effectively hardcoded, not dynamic, would be a pain to add more filter options).

public static class AgeGroups
{
    public static Dictionary<string, int> Items = new Dictionary<string, int>(){
        { "Modern (Less than 10 years old)", 1 },
        { "Retro (10 - 20 years old)", 2 },
        { "Vintage(20 - 70 years old)", 3 },
        { "Antique(70+ years old)", 4 }
    };

    public static IQueryable<ProductDTO> FilterAgeByGroup(IQueryable<ProductDTO> query, List<string> filters)
    {
        var values = new List<int>();
        var currentYear = DateTime.UtcNow.Year;
        foreach (var key in filters)
        {
            var matchingValue = Items.TryGetValue(key, out int value);

            if (matchingValue)
            {
                values.Add(value);
            }
        }

        if (Utility.EqualsIgnoringOrder(values, new List<int> { 1 }))
        {
            query = query.Where(x => x.YearManufactured >= currentYear - 10);
        }
        else if (Utility.EqualsIgnoringOrder(values, new List<int> { 2 }))
        {
            query = query.Where(x => x.YearManufactured <= currentYear - 10 && x.YearManufactured >= currentYear - 20);
        }
        else if (Utility.EqualsIgnoringOrder(values, new List<int> { 3 }))
        {
            query = query.Where(x => x.YearManufactured <= currentYear - 20 && x.YearManufactured >= currentYear - 70);
        }
        else if (Utility.EqualsIgnoringOrder(values, new List<int> { 4 }))
        {
            query = query.Where(x => x.YearManufactured <= currentYear - 70);
        }
        else if (Utility.EqualsIgnoringOrder(values, new List<int> { 1, 2}))
        {
            query = query.Where(x => x.YearManufactured >= currentYear - 20);
        }
        else if (Utility.EqualsIgnoringOrder(values, new List<int> { 1, 3 }))
        {
            query = query.Where(x => x.YearManufactured >= currentYear - 10 || (x.YearManufactured <= currentYear - 20 && x.YearManufactured >= currentYear - 70));
        }
        else if (Utility.EqualsIgnoringOrder(values, new List<int> { 1, 4 }))
        {
            query = query.Where(x => x.YearManufactured >= currentYear - 10 ||  x.YearManufactured <= currentYear - 70);
        }
        else if (Utility.EqualsIgnoringOrder(values, new List<int> { 2, 3 }))
        {
            query = query.Where(x => x.YearManufactured <= currentYear - 10 && x.YearManufactured >= currentYear - 70);
        }
        else if (Utility.EqualsIgnoringOrder(values, new List<int> { 2, 4 }))
        {
            query = query.Where(x => (x.YearManufactured <= currentYear - 10 && x.YearManufactured >= currentYear - 20) 
                                     || x.YearManufactured <= currentYear - 70);
        }
        else if (Utility.EqualsIgnoringOrder(values, new List<int> { 1, 2, 3 }))
        {
            query = query.Where(x => x.YearManufactured >= currentYear - 70);
        }
        else if (Utility.EqualsIgnoringOrder(values, new List<int> { 1, 2, 4 }))
        {
            query = query.Where(x => x.YearManufactured >= currentYear - 20 || x.YearManufactured <= currentYear - 70);
        }
        else if (Utility.EqualsIgnoringOrder(values, new List<int> { 2, 3, 4}))
        {
            query = query.Where(x => x.YearManufactured <= currentYear - 10);
        }
        else if (Utility.EqualsIgnoringOrder(values, new List<int> { 1, 3, 4}))
        {
            query = query.Where(x => x.YearManufactured >= currentYear - 10 || x.YearManufactured <= 20);
        }
        else if (Utility.EqualsIgnoringOrder(values, new List<int> { 1, 2, 3, 4 }))
        {
            // all
        }
        return query;
    }
}
Aleks Andreev
  • 7,016
  • 8
  • 29
  • 37
aspirant_sensei
  • 1,568
  • 1
  • 16
  • 36
  • Here you go: https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/concepts/expression-trees/#creating-expression-trees-by-using-the-api – tukaef Feb 14 '19 at 19:08
  • [LINQKit](https://github.com/scottksmith95/LINQKit) is a good tool for helping with this, but if your conditions are all anded together, you can simply chain the `Where` results for each sub-filter. – NetMage Feb 14 '19 at 22:04

4 Answers4

8

I've recently run into this issue myself. Through the help of another question on SO I found http://www.albahari.com/nutshell/predicatebuilder.aspx. Basically you want to build a predicate and pass that into the where clause of your query.

public static Expression<Func<T, bool>> Or<T>(this Expression<Func<T, bool>> where1, 
     Expression<Func<T, bool>> where2)
{
    InvocationExpression invocationExpression = Expression.Invoke(where2, 
         where1.Parameters.Cast<Expression>());
    return Expression.Lambda<Func<T, bool>>(Expression.OrElse(where1.Body, 
         invocationExpression), where1.Parameters);
}

public static IQueryable<ProductDTO> FilterAgeByGroup(IQueryable<ProductDTO> query,  
   List<string> filters, int currentYear)
{
    var values = new HashSet<int>();
    //Default value
    Expression<Func<ProductDTO, bool>> predicate = (ProductDTO) => false;

    foreach (var key in filters)
    {
        var matchingValue = Items.TryGetValue(key, out int value);

        if (matchingValue)
        {
            values.Add(value);
        }
    }

    if (values.Count == 0)
        return query;

    if (values.Contains(1))
    {
        predicate = predicate.Or(x => x.YearManufactured >= currentYear - 10);
    }

    if (values.Contains(2))
    {
        predicate = predicate.Or(x => x.YearManufactured <= currentYear - 10 && 
            x.YearManufactured >= currentYear - 20);
    }

    if (values.Contains(3))
    {
        predicate = predicate.Or(x => x.YearManufactured <= currentYear - 20 && 
            x.YearManufactured >= currentYear - 70);
    }

    if (values.Contains(4))
    {
        predicate = predicate.Or(x => x.YearManufactured <= currentYear - 70);
    }

    return query.Where(predicate);
}
blander
  • 96
  • 3
  • Thanks a lot, the article made sense to me and this solution leverages my existing code and doesn't require any third party libraries. – aspirant_sensei Feb 19 '19 at 14:13
3

It looks like you faced combinatorial explosion. You may declare simple cases statically with modified Items collection:

static Dictionary<string, Expression<Func<int, int, bool>>> Items
   = new Dictionary<string, Expression<Func<int, int, bool>>>
{
    {
      "Modern (Less than 10 years old)",
      (yearManufactured, currentYear) => yearManufactured >= currentYear - 10
    },
    {
      "Retro (10 - 20 years old)",
      (yearManufactured, currentYear) => yearManufactured <= currentYear - 10 && yearManufactured >= currentYear - 20
    },
    {
      "Vintage(20 - 70 years old)",
      (yearManufactured, currentYear) => yearManufactured <= currentYear - 20 && yearManufactured >= currentYear - 70
    },
    {
      "Antique(70+ years old)",
      (yearManufactured, currentYear) => yearManufactured <= currentYear - 70
    }
};

Now you can dynamically combine your simple cases with Linq Expression OrElse. Try this code:

public static IQueryable<ProductDTO> FilterAgeByGroup(
  IQueryable<ProductDTO> query, List<string> filters)
{
    var conditions = new List<Expression>();
    foreach (var key in filters)
        if (Items.TryGetValue(key, out Expression<Func<int, int, bool>> value))
            conditions.Add(value);

    // return as is if there no conditions
    if (!conditions.Any())
        return query;

    var x = Expression.Parameter(typeof(ProductDTO), "x");
    var yearManufactured = Expression.PropertyOrField(x, "YearManufactured");
    var currentYear = Expression.Constant(DateTime.UtcNow.Year);
    var body = conditions.Aggregate(
        (Expression) Expression.Constant(false), // ignore item by default
        (c, n) => Expression.OrElse(c, Expression.Invoke(n, yearManufactured, currentYear)));

    var lambda = Expression.Lambda<Func<ProductDTO, bool>>(body, x);
    return query.Where(lambda);
}
Aleks Andreev
  • 7,016
  • 8
  • 29
  • 37
  • I like this approach a lot because you're keeping the predicate mapped to the dictionary for an easy point of reference. The only problem is I do need to utilise the value (i.e the int) for other parts of my application – aspirant_sensei Feb 19 '19 at 14:10
  • @KrishanPatel You can create dictionary `Dictionary>` and with linq method `ToDictionary` modify type of value to get original dictionary from question – Aleks Andreev Feb 19 '19 at 14:14
  • Do LINQ providers support `InvocationExpression`? I seem to recall that EF6 doesn't support it. – Zev Spitz Feb 19 '19 at 23:44
  • But this can be done without `Expression.Invoke` -- see [my answer](https://stackoverflow.com/a/54777179/111794). – Zev Spitz Feb 20 '19 at 00:49
1

Using LINQKit, you can easily combine the predicates. Also, there is no reason to translate the filters List to another List just to process them once you can combine them, you just add each filter passed in.

public static class AgeGroups {
    public static Dictionary<string, int> Items = new Dictionary<string, int>(){
        { "Modern (Less than 10 years old)", 1 },
        { "Retro (10 - 20 years old)", 2 },
        { "Vintage(20 - 70 years old)", 3 },
        { "Antique(70+ years old)", 4 }
    };

    public static IQueryable<ProductDTO> FilterAgeByGroup(IQueryable<ProductDTO> query, List<string> filters) {
        var currentYear = DateTime.UtcNow.Year;

        var pred = PredicateBuilder.New<ProductDTO>();
        foreach (var fs in filters) {
            if (Items.TryGetValue(fs, out var fv)) {
                switch (fv) {
                    case 1:
                        pred = pred.Or(p => currentYear-p.YearManufactured < 10);
                        break;
                    case 2:
                        pred = pred.Or(p => 10 <= currentYear-p.YearManufactured && currentYear-p.YearManufactured  <= 20);
                        break;
                    case 3:
                        pred = pred.Or(p => 20 <= currentYear-p.YearManufactured && currentYear-p.YearManufactured  <= 70);
                        break;
                    case 4:
                        pred = pred.Or(p => 70 <= currentYear-p.YearManufactured);
                        break;
                }
            }
        }

        return query.Where(pred);
    }
}
NetMage
  • 26,163
  • 3
  • 34
  • 55
  • I like this approach as LINQKit looks like a useful library, and it keeps the code easier to read. However I would rather not include a separate library for only needing to use one feature, but thank you for bringing LINQKit to my attention. – aspirant_sensei Feb 19 '19 at 14:13
  • @KrishanPatel You can create your own version of LINQKit's `Or` operator pretty easily, though the start value handling is a tiny bit tricky. – NetMage Feb 19 '19 at 16:22
1

I would suggest dynamically constructing the expression you're going to pass into the Where (as in AlexAndreev's answer; but without using any compiler-generated expressions, only the factory methods in System.Linq.Expressions.Expression.

Define first your original dictionary with a value tuple of minimum and maximum ages for each criterion:

// using static System.Linq.Expressions.Expression

public static Dictionary<string, (int code, int? min, int? max)> Items = new Dictionary<string, (int code, int? min, int? max)>(){
    { "Modern (Less than 10 years old)", (1, null, 10) },
    { "Retro (10 - 20 years old)", (2, 10, 20) },
    { "Vintage(20 - 70 years old)", (3, 20, 70) },
    { "Antique(70+ years old)", (4, 70, null) }
};

Then you can dynamically build the predicate, adding conditions based on the passed-in filters and their matching criteria:

public static IQueryable<ProductDTO> FilterAgeByGroup(
    IQueryable<ProductDTO> query, 
    List<string> filters)
{
    var criteria = filters
        .Select(filter => {
            Items.TryGetValue(filter, out var criterion);
            return criterion; // if filter is not in Items.Keys, code will be 0
        })
        .Where(criterion => criterion.code > 0) // excludes filters not matched in Items
        .ToList();

    if (!criteria.Any()) { return query; }

    var type = typeof(ProductDTO);
    var x = Parameter(t);

    // creates an expression that represents the number of years old this ProductDTO is:
    // 2019 - x.YearManufactured
    var yearsOldExpr = Subtract(
        Constant(DateTime.UtcNow.Year),
        Property(x, t.GetProperty("YearManufactured"))
    );

    var filterExpressions = new List<Expression>();

    foreach (var criterion in criteria) {
        Expression minExpr = null;
        if (criterion.min != null) {
            // has to be at least criteria.min years old; eqivalent of:
            // 2019 - x.YearManufactured >= 10
            minExpr = GreaterThanOrEqual(
                yearsOldExpr,
                Constant(criterion.min)
            );
        }

        Expression maxExpr = null;
        if (criterion.max != null) {
            // has to be at least criteria.max years old; equivalent of
            // 2019 - x.YearManufactured <= 20
            maxExpr = LessThanOrEqual(
                yearsOldExpr,
                Constant(criterion.max)
            )
        }

        if (minExpr != null && maxExpr != null) {
            filterExpressions.Add(AndAlso(minExpr, maxExpr));
        } else {
            filterExpressions.Add(minExpr ?? maxExpr); // They can't both be null; we've already excluded that possibility above
        }
    }

    Expression body = filterExpressions(0);
    foreach (var filterExpression in filterExpressions.Skip(1)) {
        body = OrElse(body, filterExpression);
    }
    return query.Where(
        Lambda<Func<ProductDTO, bool>>(body, x)
    );
}
Zev Spitz
  • 13,950
  • 6
  • 64
  • 136