Filtering data with multiple if conditions

Question

I am trying to order data with a parameter condition name. The code works but it reads very badly and i was wondering if there is a nicer way to write it. Thanks for the help.

    public async Task<ActionResult<IEnumerable<Product>>> FilterAsync(string conditionName)
    {
        var values = await GetProducts();

        IOrderedEnumerable<Product> dataSorted =null;
        if (conditionName == "TitleASC")
        {
            dataSorted = values.OrderBy(c => c.Title);
        }

        if (conditionName == "TitleDESC")
        {
            dataSorted = values.OrderByDescending(c => c.CategoriesTitle);
        }

        if (conditionName == "DateTimeASC")
        {
            dataSorted =values.OrderBy(c => c.DateTime);
        }

        if (conditionName == "DateTimeDESC")
        {
            dataSorted = values.OrderByDescending(c => c.DateTime);
        }

        if (conditionName == "CategoryASC")
        {
            dataSorted = values.OrderBy(c => c.CategoriesTitle);
        }
        if (conditionName == "CategoryDESC")
        {
            dataSorted = values.OrderByDescending(c => c.CategoriesTitle);
        }

        return Ok(dataSorted);
    }

Answer 1

Important disclaimer

There is always a tradeoff in computer science.

The approach to this problem that I'm going to suggest requires to increase the level of engineering of your solution. Doing so is not always the right approach, because it increases the level of abstraction of your code.

If this code is not going to change frequently and the number of possible sorting criteria is small and you think that it is going to remain small, you can live happy and safe with your current implementation (or the equivalent solution based on switch statement, which is probably better in terms of code readability).

Why using an if cascade is a problem

Apart from the code readability problem, the main issue with a long cascade of if statements is a violation of the open closed principle. Here you can find an explanation of this principle.

To make a long story short, the idea is that code like the one you showed is subject to constant changes, because each time you introduce a new condition (or you remove an existing one) you have to modify the existing code by adding (or removing) a new if statement. This is not a desirable scenario because changing existing code which works can lead to bugs and is an expensive operation in terms of time and focus. The desired scenario is having a piece of code which works fine, is well tested and can remain stable over the time. In order to fullfil new requirements we would like to write new modules (for instance, new classes) without touching any of the existing and working modules.

Is changing the if cascade to a switch statement a solution ?

No it isn't. Converting the if cascade to a switch could improve the code readability maybe, but it is not the soultion of the underlying problem of violating the open closed principle. The maintenance of the switch statement is affected by the same problem as the ones explained for the cascade of if statements.

Design patterns to the rescue.

A first approach is trying to apply the following design pattern, which is basically the strategy pattern.

Notice that using this approach you extend the functionality of your program just by writing a new implementation of an interface, but you never have to modify the existing interface implementations and the code consuming the interface.

// define an interface encapsualting the desired behavior

IProductsProvider 
{
  bool CanHandleCondition(string condition);
  Task<IEnumerable<Products>> GetProducts(string condition);
}

// write an implementation of your interface for each possible condition that you have

class TitleAscendingProvider : IProductsProvider 
{
  private readonly IApiClient _apiClient;

  public TitleAscendingProvider(IApiClient apiClient)
  {
    _apiClient = apiClient ?? throw new ArgumentNullException(nameof(apiClient));
  }

  public bool CanHandleCondition(string condition) => condition == "TitleASC";

  public async Task<IEnumerable<Products>> GetProducts(string condition)
  {
    var products = await _apiClient.GetProducts();
    return products.OrderBy(c => c.Title);
  }
}

class TitleDescendingProvider : IProductsProvider 
{
  private readonly IApiClient _apiClient;

  public TitleAscendingProvider(IApiClient apiClient)
  {
    _apiClient = apiClient ?? throw new ArgumentNullException(nameof(apiClient));
  }

  public bool CanHandleCondition(string condition) => condition == "TitleDESC";

  public async Task<IEnumerable<Products>> GetProducts(string condition)
  {
    var products = await _apiClient.GetProducts();
    return products.OrderByDescending(c => c.CategoriesTitle);
  }
}

// this is the implementation of the interface that you will register with your DI container
// inject all the other implementations of the IProductsProvider interface

class CompositeProvider : IProductsProvider 
{
  private readonly IProductsProvider[] _providers;

  public TitleAscendingProvider(IEnumerable<IProductsProvider> providers)
  {
    if (providers is null) 
    {
      throw new ArgumentNullException(nameof(providers));
    }

    _providers = providers.ToArray();
  }

  public bool CanHandleCondition(string condition) => _providers.Any(p => p.CanHandleCondition(condition));

  public Task<IEnumerable<Products>> GetProducts(string condition)
  {
    var provider = _providers.FirstOrDefault(p => p.CanHandleCondition(condition));
    if (provider == null) 
    {
      throw new InvalidOperationException("Unable to find a proper provider for the condition '{condition}'")
    }

    return provider.GetProducts(condition);
  }
}

// remember to register the class CompositeProvider as the implementation of the interface IProductsProvider in the DI container
// let the DI container to inject the implementation of IProductsProvider in your controller

private readonly IProductsProvider _provider;

public async Task<ActionResult<IEnumerable<Product>>> FilterAsync(string conditionName)
{
    var products = await _provider.GetProducts(conditionName);
    return Ok(products);
}

Another possible approach based on expression trees

Another possible approach is using expression trees. An expression tree is basically an object representing some code that you can inspect at runtime, in order to analyze it and do something useful to it. One of the things that you can do given an expression tree is compiling it: by doing so you get a delegate that you can execute. In other words, you started from an object representing some code and you ended up with a delegate that you can invoke in order to execute the code represented by the expression tree.

The basic idea of this approach is asking the user the name of the field that he wants to use to sort the products and the sorting direction (ascending or descending). Then you can build an expression tree representing some code that, given an instance of the product class, access the property that must be used to sort the products. Finally you can compile the expression tree in order to get a delegate instance that you can pass to the LINQ to object OrderBy extension method.

This solution is not fully applicable to your problem because it requires to invoke a generic method and you can't infer the generic type argument from the input coming from the user (the name of the property used to sort the products), but I think that expression trees are worth mentioning here. Consider this part of my answer as a trigger to further investigations in the world of expression trees.

The code which follows is an example of using an expression tree to get the delegate used to sort some objects, starting by the name of the property used to sort.

static class Program
  {
    public static void Main(string[] args)
    {
      var people = new Person[]
      {
        new Person { Name = "Bob", Age = 11 },
        new Person { Name = "Alice", Age = 8 },
        new Person { Name = "Tony", Age = 1 }
      };

      //imagine this comes from the user...
      var propertyName = nameof(Person.Age);          

      // the issue is that here you need to pass the right generic type argument and based on my understanding you can't infer it from the propertyName variable
      var expression = GetPropertyExpression<int>(propertyName);
      var func = expression.Compile();

      var sorted = people.OrderBy(func).ToArray();
    }

    private static Expression<Func<Person, T>> GetPropertyExpression<T>(string propertyName)
    {
      var parameter = Expression.Parameter(typeof(Person), "model");
      var property = Expression.Property(parameter, propertyName);
      var expression = Expression.Lambda<Func<Person, T>>(property, parameter);
      return expression;
    }

    public class Person 
    {
      public string Name { get; set; }
      public int  Age { get; set; }
    }
  }

Any community member having a better understanding of me in the expression tree topic is welcome to improve this idea or to explain if and why expression trees are not suitable for the scenario of this question.

Here you can find another example of building a dynamic query by using an expression tree.

Edit 26th April 2020

As pointed out by other answers to the same questions it's worth the effort giving a look to the System.Linq.Dynamic.Core library which allows to create dynamic queries which are text based.

This is probably the safest way to employ expression trees in order to solve your problem. Unless you are an expert in the expression tree subject it's always better to use proper libraries and avoid going to production with homemade "poor man" solutions.

Answer 2

You can simplify you solution like this:

public async Task<ActionResult<IEnumerable<Product>>> FilterAsync(string 
conditionName)
{
    var values = await GetProducts();
    IOrderedEnumerable<Product> dataSorted =null;

    switch (conditionName)
    {
        case "TitleASC":
            dataSorted = values.OrderBy(c => c.Title);
            break;
        case "TitleDESC":
            dataSorted = values.OrderByDescending(c => c.CategoriesTitle);
            break;
        case "DateTimeASC":
            dataSorted =values.OrderBy(c => c.DateTime).ThenBy(c => c.CategoriesTitle);
            break;
        case "DateTimeDESC":
            dataSorted = values.OrderByDescending(c => c.DateTime).ThenByDescending(c => c.CategoriesTitle);
            break;
        default:
            // if you want you can add default order here
            break;
    }
    return Ok(dataSorted);
}