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;
 }

• Phrancis: Welcome to Code Review! I hope you get some great answers!

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.