Reduce number of conditional operators(4) used in the expression (maximum allowed 3) in Expression<Func<T,bool>predicate

Question

I am using a the below function to fetch data from mongoDB:

Task<IEnumerable<T>> ReadDocuments(Expression<Func<T,bool>> predicate)

There are multiple conditions based on which I need to filter the data and I am using the function like so:

cars = await this.carRepository.ReadDocuments(car => car.no == no && 
car.engine == engine && car.model == model && car.color == color && car.country==country)

For the above code, I am facing a SonarQube issue:

Reduce number of conditional operators(4) used in the expression (maximum allowed 3) Expressions should not be too complex

The documentation link for the issue, which was not very useful for my case

How can I split the conditions in Expression<Func<T,bool> predicate to reduce the number of conditional operators in the statement?

Answer 1

I recommend that you ignore this "maintainability" warning when applied to LINQ query expressions. According to the docs, SonarQube recommends simplifying complex conditionals by extracting portions into methods, e.g.:

if (((condition1 && condition2) || (condition3 && condition4)) && condition5) { ... }

Would become:

if ((MyFirstCondition() || MySecondCondition()) && MyLastCondition()) { ... }

However, you cannot adopt this solution for query expressions passed to LINQ database providers because database providers generally cannot translate arbitrary Expression.Invoke or Expression.Call .NET method calls into database queries. See e.g.

• Create expression tree on parent object from existing expression for c# mongodb driver parsing
• Method cannot be translated into a store expression
• Calling functions in a LINQ query

As a workaround, since you really do need to run a query with all five conditionals, I suppose you could create some extension method that combines multiple predicate expressions using AndAlso expressions:

public static class ExpressionExtensions
{
    public static  Expression<Func<T,bool>> AndAlsoAll<T>(this Expression<Func<T,bool>> expression, params Expression<Func<T,bool>> [] others)
    {
        var body = expression.Body;
        var parameter = expression.Parameters[0];
        foreach (var other in others)
            body = Expression.AndAlso(body, new ParameterReplacer((other.Parameters[0], (Expression)parameter)).Visit(other.Body));
        return Expression.Lambda<Func<T, bool>>(body, parameter);
    }
}

class ParameterReplacer : ExpressionVisitor
{
    // Replace formal parameters (e.g. of a lambda body) with some containing expression in scope.
    readonly Dictionary<ParameterExpression, Expression> parametersToReplace;
    public ParameterReplacer(params (ParameterExpression parameter, Expression replacement) [] parametersToReplace) =>
        this.parametersToReplace = parametersToReplace.ToDictionary(p => p.parameter, p => p.replacement);
    protected override Expression VisitParameter(ParameterExpression p) => 
        parametersToReplace.TryGetValue(p, out var e) ? e : base.VisitParameter(p);
}

Then add some method (or extension method) to your carRepository type such as:

public Task<IEnumerable<T>> ReadDocumentsWhereAll(Expression<Func<T,bool>> predicate, params Expression<Func<T,bool>> [] andAlsoAllPredicates)
{
    return ReadDocuments(predicate.AndAlsoAll(andAlsoAllPredicates));
}

And now you will be able to do something like:

var cars = await this.carRepository.ReadDocumentsWhereAll(car => car.no == no, 
                                                          car => car.engine == engine, 
                                                          car => car.model == model, 
                                                          car => car.color == color, 
                                                          car => car.country==country);

But honestly I don't think that makes the code any more readable or maintainable (and even makes the short-circuiting behavior we expect from && expressions less obvious).

Mockup fiddle here.