Chaining multiple predicates

Question

I have a some filters that chains around 30 Expression<Func<T, bool>> using linq to entities. Currently this is how I am managing them

 ...
 //project filter
 Expression<Func<Project, bool>> projectFilter = FilterEnabled();
 projectFilter = projectFilter.And(GetProjectByOrganization())
 .And(GetProjectByProductLine())
 .And(GetProjectByProjectType())
 ...
 //subproject filter
 Expression<Func<SubProject, bool>> subProjectFilter = FilterEnabled();
 subProjectFilter = subProjectFilter .And(...)
 ...
 //activity filter
 Expression<Func<Activity, bool>> activityFilter = FilterEnabled();
 activityFilter = activityFilter .And(...)
 ...

The problem is .And(Expression<Func<T, bool>>) extension method goes on for another 30 lines or so. How can I manage this another way instead of having to add .And 30+ times for each filter critera. Each filter method looks something like this.

public Expression<Func<Project, bool>> GetProjectByProjectId()
{
 return prj => FilterCriteria.ProjectId == null || prj.ProjectID == FilterCriteria.ProjectId.Value;
}

This is also my predicate builder class where I created the .And()

 public static class PredicateBuilder
 {
 public static Expression<Func<T, bool>> And<T>(this Expression<Func<T, bool>> a, Expression<Func<T, bool>> b)
 {
 ParameterExpression p = a.Parameters[0];
 SubstExpressionVisitor visitor = new SubstExpressionVisitor();
 visitor.subst[b.Parameters[0]] = p;
 Expression body = Expression.And(a.Body, visitor.Visit(b.Body));
 return Expression.Lambda<Func<T, bool>>(body, p);
 }
 public static Expression<Func<T, bool>> Or<T>(this Expression<Func<T, bool>> a, Expression<Func<T, bool>> b)
 {
 ParameterExpression p = a.Parameters[0];
 SubstExpressionVisitor visitor = new SubstExpressionVisitor();
 visitor.subst[b.Parameters[0]] = p;
 Expression body = Expression.Or(a.Body, visitor.Visit(b.Body));
 return Expression.Lambda<Func<T, bool>>(body, p);
 }
 }
 internal class SubstExpressionVisitor : System.Linq.Expressions.ExpressionVisitor
 {
 public Dictionary<Expression, Expression> subst = new Dictionary<Expression, Expression>();
 protected override Expression VisitParameter(ParameterExpression node)
 {
 Expression newValue;
 if (subst.TryGetValue(node, out newValue))
 {
 return newValue;
 }
 return node;
 }
 }

• Denis: Is performance a concern? I can easily see that becoming a heavy operation.
• Train: @Denis No it's not.

Answer 1

To answer the direct question, the obvious way is to have an All method which takes a params Expression<Func<T, bool>>[] expressions argument. You'll still have a 30-line call, but you won't have And on each line. There are potential side-benefits:

• It should reduce the amount of wrapping an expression just to unwrap it immediately.
• If you decide that you want AndAlso instead of And (is there any reason for not using it?) then it's much simpler to change.

---

I would also be tempted to favour a refactor which allows null expressions. If instead of

public Expression<Func<Project, bool>> GetProjectByProjectId()
{
 return prj => FilterCriteria.ProjectId == null || prj.ProjectID == FilterCriteria.ProjectId.Value;
}

you have

public Expression<Func<Project, bool>> GetProjectByProjectId()
{
 return FilterCriteria.ProjectId == null ? null : (prj => prj.ProjectID == FilterCriteria.ProjectId.Value);
}

(with whatever type coercion is necessary to make that compile - I haven't tested it) then you can filter out null expressions in All. This both simplifies the expression constructed and the SQL generated from it, which might improve performance and might also make debugging it easier.

---

Finally, two comments about SubstExpressionVisitor:

 public Dictionary<Expression, Expression> subst = new Dictionary<Expression, Expression>();

On the basis of coding to the interface rather than the implementation, the type should be IDictionary<Expression, Expression>. I personally would also prefer to hide the field and take the substitutions as arguments to the constructor rather than by modifying a public field after construction, but that's a subjective issue of style. If making that change, the type can become IReadOnlyDictionary<Expression, Expression> instead.

 protected override Expression VisitParameter(ParameterExpression node)
 {
 Expression newValue;
 if (subst.TryGetValue(node, out newValue))
 {
 return newValue;
 }
 return node;
 }

Too many people use Contains and then [key] rather than TryGetValue, so thumbs up for getting that right. I would observe, though, that foo.TryGetValue(key, out val) ? val : defaultVal is a common enough pattern that you could consider factoring it out as an extension method to IDictionary<K,V> and IReadOnlyDictionary<K,V>.