Return lambda expression based on parameters

Question

This function returns a lambda expression based on the parameters sent. I am sure there is a better way to do this.

    [NonAction]
    private Expression<Func<ScheduleItemDto, bool>> Where(Guid siteGuid, Guid? eventGuid, MonthOfYear month, int year, bool? closed)
    {
        Expression<Func<ScheduleItemDto, bool>> notAdminExpression = x => x.Active == true;
        Expression<Func<ScheduleItemDto, bool>> baseExpression = x => x.SiteGuid == siteGuid && x.Date.Month == (int)month + 1 && x.Date.Year == year;
        Expression<Func<ScheduleItemDto, bool>> eventExpression = x => x.EventGuid == eventGuid;
        Expression<Func<ScheduleItemDto, bool>> closedExpression = x => x.Closed != null;
        Expression<Func<ScheduleItemDto, bool>> notClosedExpression = x => x.Closed == null;
        Expression<Func<ScheduleItemDto, bool>> resultExpression = x => true;

        switch (closed)
        {
            case null:
                if (eventGuid == null)
                {
                    resultExpression = baseExpression;                      
                }
                else
                {
                    resultExpression = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, eventExpression.Body), baseExpression.Parameters[0]);
                }
                break;

            case true:
                if (eventGuid == null)
                {
                    resultExpression = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, closedExpression.Body), baseExpression.Parameters[0]);
                }
                else
                {
                    Expression<Func<ScheduleItemDto, bool>> exp = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, closedExpression.Body), baseExpression.Parameters[0]);
                    resultExpression = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(exp.Body, eventExpression.Body), exp.Parameters[0]);
                }
                break;

            case false:
                if (eventGuid == null)
                {
                    resultExpression = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, notClosedExpression.Body), baseExpression.Parameters[0]);
                }
                else
                {
                    Expression<Func<ScheduleItemDto, bool>> exp = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, notClosedExpression.Body), baseExpression.Parameters[0]);
                    resultExpression = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(exp.Body, eventExpression.Body), exp.Parameters[0]);
                }
                break;
        }

        if (!AccessControl.HasAccess(Permissions.DashboardAdministrator))
        {
            return Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(resultExpression.Body, notAdminExpression.Body), resultExpression.Parameters[0]);
        }

        return resultExpression;
    }

Answer 1

Personally, I find your code easy enough to follow. The flow control is lengthy but is quite simple. A few notes before searching alternatives for the flow control.

---

Expression<Func<ScheduleItemDto, bool>> notAdminExpression = x => x.Active == true;

Could be

Expression<Func<ScheduleItemDto, bool>> notAdminExpression = x => x.Active;

---

Your function could be broken in two pieces, one for the switch statement and another for the permission part. It may be a matter of personal preference, but allows early return statements, removing the assignment operation and thus entirely removing any state from the function, pure functions are easier to follow.

// ...
case null:
    if (eventGuid == null)
    {
        return baseExpression;                      
    }
    else
    {
        return Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, eventExpression.Body), baseExpression.Parameters[0]);
    }
// ...

private Expression<Func<ScheduleItemDto, bool>> RestrictPermission(Expression<Func<ScheduleItemDto, bool>> baseExpression)
{
    Expression<Func<ScheduleItemDto, bool>> notAdminExpression = x => x.Active
    return Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, notAdminExpression.Body), baseExpression.Parameters[0]);
}
// ...
RestrictPermission(Where(...));

---

Now the ways I can think of rewriting the switch statement

If chain

if(closed == null && eventGuid == null)
    return ...;
else if (closed == null && eventGuid != null)
    return ...;
else if (closed && eventGuid == null)
    return ...;
...

Very easy to parse which case each return belongs, but quite verbose.

Dictionary

 var options = new Dictionary<Tuple<bool?, bool>, Expression<Func<ScheduleItemDto, bool>>> {
    { Tuple.Create(null, true), baseExpression },
    { Tuple.Create(null, false), Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, eventExpression.Body), baseExpression.Parameters[0]) },
    { Tuple.Create(true, true), Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, closedExpression.Body), baseExpression.Parameters[0]) },
    // ...
}
return options[closed, eventGuid == null];

Adds complexity and reduces readability, but is probably the most compact version.

If plus ternary

if (closed == null)
    return eventGuid == null
        ? baseExpression 
        : Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, closedExpression.Body), baseExpression.Parameters[0]);
else if (closed)
    ...
else
    ...

Half-way between readability and verbosity.

---

Honestly, I would stick to your switch or a chained-if. You can't get much elegance without making the code unnecessary complex. Or at least not with the options I know, I hope someone has a better solution.

Answer 2

I don't "get" this whole Expression syntax. I find it really hard to follow and maintain. I mean, yes, when you need to build arbitrary expression, based on some data (user-entered formula, for example) - Expression class is useful. But in your case - you know the evaluation logic beforehand. Why use expressions at all? I would drop them while its not too late. A couple of regular methods will do the same job, while being a lot more... straightforward:

    private bool CheckItem(ScheduleItemDto item, Guid siteGuid, Guid? eventGuid, MonthOfYear month, int year, bool? closed)
    {
        return CheckDate(item, siteGuid, eventGuid, month, year) 
               && CheckClosed(item, closed)
               && CheckAccess();
    }

    private bool CheckDate(ScheduleItemDto item, Guid siteGuid, Guid? eventGuid, MonthOfYear month, int year)
    {
        return item.SiteGuid == siteGuid
               && (eventGuid == null || eventGuid == item.EventGuid)
               && item.Date.Month == (int) month + 1
               && item.Date.Year == year;
    }

    private bool CheckClosed(ScheduleItemDto item, bool? closed)
    {
        if (closed == null) return true;
        return (item.Closed == null && !closed.Value) || (item.Closed != null && closed.Value);
    }

    //if you _really_ need to get an expression 
    //you can use lambdas as shown below
    private Expression<Func<ScheduleItemDto, bool>> Where(Guid siteGuid, Guid? eventGuid, MonthOfYear month, int year, bool? closed)
    {
        return x => CheckItem(x, siteGuid, eventGuid, month, year, closed);
    }

Some of those methods should probably be refactored into members (or extension methods) of ScheduleItemDto class, but its hard to tell, without knowing the details.