Two functions to impose order on queries

I am writing a c# program to build generic sort algorithm on a request to a collection of data. The data needs to be sorted based on the supplied columns.I am using Entity framework and Expression builder to achieve the required functionality.

In one of the methods I see that some code is duplicated and I am not getting any idea how I can remove the duplicate code by replacing it with some reusable code.

```
private static IQueryable SortQuery(IQueryable query, SortFilter sortFilter)
{
IOrderedQueryable orderedQuery = null;
if (sortFilter != null && !string.IsNullOrEmpty(sortFilter.SortOn))
{
var sortParam = Expression.Parameter(typeof(T), "x");
// Using ; as the delimeter for specifying multiple columns in sort criteria
string[] sortColumns = sortFilter.SortOn.Split(';');
foreach (string sortColumn in sortColumns)
{
Expression property = NestedExpressionProperty(sortParam, sortColumn);
orderedQuery = OrderQuery(orderedQuery??query, property, sortParam, sortFilter,ref orderedQuery);
}
return orderedQuery;
}
else
{
var sortParam = Expression.Parameter(typeof(T), "x");
Expression property = Expression.Property(sortParam, "Id");
var sortExpr = Expression.Lambda>(
property, new[] { sortParam });

return query.OrderBy(sortExpr);
}
}
private static IOrderedQueryable OrderQuery(IQueryable query, Expression property, ParameterExpression sortParam, SortFilter sortFilter, ref IOrderedQueryable orderedQuery)
{
if (property.Type == typeof(int))
{
var sortExpr = Expression.Lambda>(property, new[] { sortParam });
orderedQuery = !sortFilter.IsDescending
? (orderedQuery == null ? query.OrderBy(sortExpr) : orderedQuery.ThenBy(sortExpr))
: (orderedQuery == null

These are some general issues I see with your code:

Repeated Code

This block of code is repeated 4 times:

orderedQuery = !sortFilter.IsDescending 
             ? (orderedQuery == null ? query.OrderBy(sortExpr) : orderedQuery.ThenBy(sortExpr)) 
             : (orderedQuery == null ? query.OrderByDescending(sortExpr) : orderedQuery.ThenByDescending(sortExpr));

Put this into its own function/method and call it instead of copy/pasting it in 4 different places. Something like this (sorry for the pseudo-syntax):

setQueryOrderingFromSortFilter(sortFilter, sortExpr, ref orderedQuery)
{
    orderedQuery = !sortFilter.IsDescending 
             ? (orderedQuery == null ? query.OrderBy(sortExpr) : orderedQuery.ThenBy(sortExpr)) 
             : (orderedQuery == null ? query.OrderByDescending(sortExpr) : orderedQuery.ThenByDescending(sortExpr));
}

Then you can call it in each place where you have it copied right now:

if (property.Type == typeof(int))
{
    var sortExpr = Expression.Lambda//...etc.
    setQueryOrderingFromSortFilter(sortFilter, sortExpr, orderedQuery);
}
else if (property.Type == typeof(bool))
{
    var sortExpr = Expression.Lambda//..etc.
    setQueryOrderingFromSortFilter(sortFilter, sortExpr, orderedQuery);
}
// etc...

Readability

Ternary operators are compact but can be difficult to read. Nested ternary operators even more so. You're not saving any time by using them instead of if/else blocks, but you are hurting readability. Just make a simple set of 2 if/else blocks for setting the query ordering and be done with it.

Same thing with the NestedExpressionProperty() function. That return statement is very confusing. It looks at first glance to be returning a boolean!

Also, it would improve readability if you put some whitespace between your functions.

Why are you returning a reference parameter?

Your function signature is this:

private static IOrderedQueryable OrderQuery(IQueryable query, Expression property, ParameterExpression sortParam, SortFilter sortFilter, ref IOrderedQueryable orderedQuery)

The last parameter, orderedQuery, is a reference parameter. So you're actually changing the variable. But then you also return it. That seems redundant.

StackExchange Code Review Q#93383, answer score: 6