10
\$\begingroup\$

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;
    }
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! I hope you get some great answers! \$\endgroup\$ – Phrancis Aug 15 '15 at 23:55
5
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.