6
\$\begingroup\$

I am writing a piece of code for C# Web Api, letting the clients to pass a column name and sort direction as parameter. Although there are, like, 30-ish properties, so the following code (despite it works) gets ugly after a while.

What are my options to avoid repeating this seemingly same pieces of code?

if (column == nameof(MyModel.ColumnA).ToLower())
{
    if (parameters.IsOrderByAsc)
    {
        return queryResult.OrderBy(q => q.ColumnA);
    }

    return queryResult.OrderByDescending(q => q.ColumnA);
}

if (column == nameof(MyModel.ColumnB).ToLower())
{
    if (parameters.IsOrderByAsc)
    {
        return queryResult.OrderBy(q => q.ColumnB);
    }

    return queryResult.OrderByDescending(q => q.ColumnB);
}

if (column == nameof(MyModel.ColumnC).ToLower())
{
    if (parameters.IsOrderByAsc)
    {
         return queryResult.OrderBy(q => q.ColumnC);
    }

    return queryResult.OrderByDescending(q => q.ColumnC);
}

....


```
\$\endgroup\$
7
  • 2
    \$\begingroup\$ Please write an appropriate tile: codereview.stackexchange.com/help/how-to-ask \$\endgroup\$ – BCdotWEB Feb 16 at 12:39
  • \$\begingroup\$ A single switch-statement to replace each outer-if-statement and a function to do the inner-if-statement and returns should make this pretty short and simple. You could also create a dictionary where the key is the column name and the value is the expression (e.g. q => q.ColumnA). Then just look up the expression and plug it into the return value. \$\endgroup\$ – Shelby115 Feb 16 at 13:12
  • 1
    \$\begingroup\$ stackoverflow.com/a/233505/648075 ? \$\endgroup\$ – BCdotWEB Feb 16 at 14:00
  • \$\begingroup\$ You probably want to use ToLowerInvariant(), otherwise the code will behave weirdly in certain locales (e.g. in Turkey I does not lowercase to i) \$\endgroup\$ – CodesInChaos Feb 16 at 22:46
  • \$\begingroup\$ What is queryResult? An IQueryable, making the orderby go to the translated SQL? Maybe you can add a tag for the specific LINQ flavor you're using (like Entity Framework). \$\endgroup\$ – Gert Arnold Feb 17 at 13:34
1
\$\begingroup\$

If you don't want to pull in DynamicLinq its not a lot of Expression tree work to get exactly what OP is looking for. In the constructor getting the methods we need to call for the Queryable, I'm assuming this is IQueryable and not IEnumerable. If IEnumerable just swap all the Queryable for Enumerable. For the ToLower matching of the property using the BindingFlag.IgnoreCase. Also threw in ThenBy as a bonus as wasn't a lot of work.

public static class QueryableExtensions
{
    private static readonly MethodInfo _orderBy;
    private static readonly MethodInfo _orderByDescending;
    private static readonly MethodInfo _thenBy;
    private static readonly MethodInfo _thenByDescending;

    static QueryableExtensions()
    {
        Func<IQueryable<object>, Expression<Func<object, object>>, IOrderedQueryable<object>> orderBy = Queryable.OrderBy;
        _orderBy = orderBy.Method.GetGenericMethodDefinition();
        orderBy = Queryable.OrderByDescending;
        _orderByDescending = orderBy.Method.GetGenericMethodDefinition();
        Func<IOrderedQueryable<object>, Expression<Func<object, object>>, IOrderedQueryable<object>> thenBy = Queryable.ThenBy;
        _thenBy = thenBy.Method.GetGenericMethodDefinition();
        thenBy = Queryable.ThenByDescending;
        _thenByDescending = thenBy.Method.GetGenericMethodDefinition();
    }

    public static IOrderedQueryable<TSource> OrderBy<TSource>(this IQueryable<TSource> source, string propertyName, bool orderByAscending = true)
    {
        return CreateExpression(source, propertyName, orderByAscending ? _orderBy : _orderByDescending);
    }

    public static IOrderedQueryable<TSource> ThenBy<TSource>(this IOrderedQueryable<TSource> source, string propertyName, bool orderByAscending = true)
    {
        return CreateExpression(source, propertyName, orderByAscending ? _thenBy : _thenByDescending);
    }
    
    private static IOrderedQueryable<TSource> CreateExpression<TSource>(IQueryable<TSource> source, string propertyName, MethodInfo method)
    {
        var propInfo = typeof(TSource).GetProperty(propertyName, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance) ?? throw new ArgumentException(nameof(propertyName));
        var methodInfo = method.MakeGenericMethod(typeof(TSource), propInfo.PropertyType);
        var sourceParam = Expression.Parameter(typeof(TSource), "source");
        var property = Expression.Property(sourceParam, propInfo);
        var lambda = Expression.Lambda(typeof(Func<,>).MakeGenericType(typeof(TSource), propInfo.PropertyType), property, sourceParam);
        var call = Expression.Call(methodInfo, source.Expression, lambda);
        return (IOrderedQueryable<TSource>)source.Provider.CreateQuery<TSource>(call);
    }
}

Now can just call it like return queryResult.OrderBy(column, parameters.IsOrderByAsc)

This doesn't do nested properties but wouldn't be much work to make that w

\$\endgroup\$
5
\$\begingroup\$

If you're willing to use the DynamicLinq library, the problem becomes trivial. It allows you to pass a string parameter to the OrderBy method, bypassing your entire logic:

queryResult.OrderBy(column)

If this is related to doing a SQL database lookup, if you think about it, the issue you're facing it a bit of a self-imposed issue, because you start from a string value and you're eventually going to generate a string value (in the SQL query). The issue you're facing is that standard LINQ forces you to map it to an actual property of the entity, which in this case is an unnecessary mapping which DynamicLinq allows you to bypass.

If you want, you could add a bit of validation to confirm that the passed string is a known property of your entity type.

bool columnIsProperty = typeof(MyModel)
                           .GetProperties()
                           .Any(prop => prop.Name.ToLower() == column.ToLower());

if(columnIsProperty)
    queryResult = queryResult.OrderBy(column);
\$\endgroup\$
3
  • \$\begingroup\$ Change typeof(MyModel).GetType().GetProperties() to typeof(MyModel).GetProperties(). Also it can be cached to some field because Reflection operations aren't enough fast at all. \$\endgroup\$ – aepot Feb 16 at 16:24
  • \$\begingroup\$ @aepot Good catch, I had adapted a different piece of code and forgot to omit the GetType(). Caching might be relevant for frequent operations on e.g. an always-on service, but in scope of e.g. a web service where you reflect once for a web request, caching means retaining state, which brings more problems than it solves. I'd be more inclined to generate a list of properties on build instead of caching it at runtime. \$\endgroup\$ – Flater Feb 16 at 16:32
  • \$\begingroup\$ web service can receive many rps (100? 1000? 10k?), btw if you you're fine with Reflection performance, allocating an array of strings is resulting in a GC job. private static readonly string[] modelPropertyNames = typeof(MyModel).GetProperties().Select(prop => prop.Name.ToLower()).ToArray() and then bool columnIsProperty = modelPropertyNames.Any(name => name == column.ToLower()). That is what means caching for me: storing few data that never changes in memory once and reuse then. Anyway it affects the performance, even for 1rps. \$\endgroup\$ – aepot Feb 16 at 16:50

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.