patterncsharpMinor

Return lambda expression based on parameters

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

expressionreturnbasedparameterslambda

Problem

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> Where(Guid siteGuid, Guid? eventGuid, MonthOfYear month, int year, bool? closed)
{
Expression> notAdminExpression = x => x.Active == true;
Expression> baseExpression = x => x.SiteGuid == siteGuid && x.Date.Month == (int)month + 1 && x.Date.Year == year;
Expression> eventExpression = x => x.EventGuid == eventGuid;
Expression> closedExpression = x => x.Closed != null;
Expression> notClosedExpression = x => x.Closed == null;
Expression> resultExpression = x => true;

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

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

case false:
if (eventGuid == null)
{
resultExpression = Expression.Lambda>(Expression.AndAlso(baseExpression.Body, notClosedExpression.Body), baseExpression.Parameters[0]);
}

Solution

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> notAdminExpression = x => x.Active == true;

Could be

Expression> 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>(Expression.AndAlso(baseExpression.Body, eventExpression.Body), baseExpression.Parameters[0]);
    }
// ...

private Expression> RestrictPermission(Expression> baseExpression)
{
    Expression> notAdminExpression = x => x.Active
    return Expression.Lambda>(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, Expression>> {
    { Tuple.Create(null, true), baseExpression },
    { Tuple.Create(null, false), Expression.Lambda>(Expression.AndAlso(baseExpression.Body, eventExpression.Body), baseExpression.Parameters[0]) },
    { Tuple.Create(true, true), Expression.Lambda>(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>(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.

Code Snippets

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

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

// ...
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(...));

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

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

Context

StackExchange Code Review Q#101071, answer score: 6

Revisions (0)

No revisions yet.