Factoring WHERE clauses in IQueryable

Question

I recovered some spaghetti code and I have to refactor it. I do not want a method with over 200 lines, for me it is not object oriented programming. I am trying to ponder on the question and would like to have your suggestions.

private static IQueryable<Trade> BuildTradeQuery(IRepositoryFactory repository, SearchCriteria SearchCriteria)
    {
        var query = GetTrades(repository);
        var dteToday = DateTime.Today;

        if (SearchCriteria.RemitterId != null)
        {
            query = query.Where(x => x.RemitterId == (Remitter) SearchCriteria.RemitterId);
        }
        if (!string.IsNullOrWhiteSpace(SearchCriteria.TradeReference))
        {
            query = query.Search(x => x.TradeReference, SearchCriteria.TradeReference);
        }
        if (!string.IsNullOrWhiteSpace(SearchCriteria.Sicovam))
        {
            query = query.Search(x => x.Sicovam, SearchCriteria.Sicovam);
        }
        if (SearchCriteria.AssetClassId != null)
        {
            query = query.Where(x => x.AssetClassId == (AssetClass) SearchCriteria.AssetClassId);
        }
        if (!string.IsNullOrWhiteSpace(SearchCriteria.Desk))
        {
            query = query.Where(x => x.Desk == SearchCriteria.Desk);
        }
        if (SearchCriteria.SubGroupUpiId != null)
        {
            query = query.Where(x => x.SubGroupUpiId == (SubGroupUpi) SearchCriteria.SubGroupUpiId);
        }
        if (SearchCriteria.IsBuy != null)
        {
            query = query.Where(x => x.IsBuy == SearchCriteria.IsBuy);
        }
        if (SearchCriteria.FromTradeDate != null)
        {
            query = query.Where(x => x.TradeDate >= SearchCriteria.FromTradeDate);
        }
        if (SearchCriteria.ToTradeDate != null)
        {
            query = query.Where(x => x.TradeDate <= SearchCriteria.ToTradeDate);
        }
        if (SearchCriteria.FromStartDate != null)
        {
            query = query.Where(x => x.StartDate >= SearchCriteria.FromStartDate);
        }
        if (SearchCriteria.ToStartDate != null)
        {
            query = query.Where(x => x.StartDate <= SearchCriteria.ToStartDate);
        }
        if (SearchCriteria.FromEndDate != null)
        {
            query = query.Where(x => x.EndDate >= SearchCriteria.FromEndDate);
        }
        if (SearchCriteria.ToEndDate != null)
        {
            query = query.Where(x => x.EndDate <= SearchCriteria.ToEndDate);
        }
        if (!string.IsNullOrWhiteSpace(SearchCriteria.UtiPrefix))
        {
            query = query.Where(x => x.UtiPrefix == SearchCriteria.UtiPrefix);
        }
        if (!string.IsNullOrWhiteSpace(SearchCriteria.UtiValue))
        {
            query = query.Where(x => x.UtiValue == SearchCriteria.UtiValue);
        }
        if (!string.IsNullOrWhiteSpace(SearchCriteria.UsiPrefix))
        {
            query = query.Where(x => x.UsiPrefix == SearchCriteria.UsiPrefix);
        }
        if (!string.IsNullOrWhiteSpace(SearchCriteria.UsiValue))
        {
            query = query.Where(x => x.UsiValue == SearchCriteria.UsiValue);
        }
        if (SearchCriteria.ReasonCodeId != null)
        {
            query = query.Where(x => x.ReasonCodeId == (ReasonCode) SearchCriteria.ReasonCodeId);
        }
        if (SearchCriteria.PutCallId != null)
        {
            query = query.Where(x => x.PutCallId == (PutCall) SearchCriteria.PutCallId);
        }
        if (SearchCriteria.FromNotional != null)
        {
            query = query.Where(x => x.Notional1 >= SearchCriteria.FromNotional);
        }
        if (SearchCriteria.ToNotional != null)
        {
            query = query.Where(x => x.Notional1 <= SearchCriteria.ToNotional);
        }
        if (SearchCriteria.CurrencyId != null)
        {
            query = query.Where(x => x.FirstCurrencyId == (Currency) SearchCriteria.CurrencyId);
        }
        if (!string.IsNullOrWhiteSpace(SearchCriteria.CounterpartMnemo))
        {
            query = query.Search(x => x.Counterpart.Mnemo, SearchCriteria.CounterpartMnemo);
        }
        if (!string.IsNullOrWhiteSpace(SearchCriteria.CounterpartLegalName))
        {
            query = query.Search(x => x.Counterpart.LegalName, SearchCriteria.CounterpartLegalName);
        }
        if (SearchCriteria.CounterpartClassificationId != null)
        {
            query =
                query.Where(
                    x =>
                        x.Counterpart.CounterpartClassificationId ==
                        (CounterpartClassification) SearchCriteria.CounterpartClassificationId);
        }

        if (SearchCriteria.IsEarlyMatured == false)
        {
            query = query.Where(x => x.IsEarlyMatured != true);
        }
        if (SearchCriteria.IsMatured == false)
        {
            query = query.Where(x => x.EndDate > dteToday);
        }
        if (SearchCriteria.IsNonMatured == false)
        {
            query = query.Where(x => x.EndDate <= dteToday);
        }

        if (SearchCriteria.PendingTrade == false)
        {
            query = query.Where(x => x.TradeStatusId != TradeStatus.Pending);
        }
        if (SearchCriteria.VerifiedTrade == false)
        {
            query = query.Where(x => x.TradeStatusId != TradeStatus.Verified);
        }
        if (SearchCriteria.CanceledTrade == false)
        {
            query = query.Where(x => x.TradeStatusId != TradeStatus.Canceled);
        }

        if (SearchCriteria.EventReference != null)
        {
            var refExpression = QueryableExtension.BuildSearchExpression<Event>(x => x.EventReference,
                SearchCriteria.EventReference);
            query =
                query.Where(
                    trade =>
                        trade.Event.AsQueryable().Any(refExpression));
        }
        if (SearchCriteria.PendingEvent == false)
        {
            query = query.Where(trade => trade.Event.All(evt => evt.EventStatusId != EventStatus.Pending));
        }
        if (SearchCriteria.VerifiedEvent == false)
        {
            query = query.Where(trade => trade.Event.All(evt => evt.EventStatusId != EventStatus.Verified));
        }
        if (SearchCriteria.CanceledEvent == false)
        {
            query = query.Where(trade => trade.Event.All(evt => evt.EventStatusId != EventStatus.Canceled));
        }
        if (SearchCriteria.EventReasonCodeId != null)
        {
            query =
                query.Where(trade => trade.Event.Any(evt => evt.ReasonCodeId == SearchCriteria.EventReasonCodeId));
        }
        var documentStatus = new List<DocumentStatus?>(Enum.GetValues(typeof(DocumentStatus)).OfType<DocumentStatus?>()) {null};
        documentStatus.Remove(DocumentStatus.Declined);
        var noFilterCount = documentStatus.Count;

        if (SearchCriteria.DocumentNoStatus == false)
        {
            documentStatus.Remove(null);
        }
        if (SearchCriteria.DocumentPending == false)
        {
            documentStatus.Remove(DocumentStatus.Pending);
        }
        if (SearchCriteria.DocumentPendingApproval == false)
        {
            documentStatus.Remove(DocumentStatus.PendingValidation);
        }
        if (SearchCriteria.DocumentApproved == false)
        {
            documentStatus.Remove(DocumentStatus.Approved);
        }
        if (documentStatus.Count != noFilterCount)
        {
            if (documentStatus.Contains(null))
            {
                query =
                    query.Where(
                        trade =>
                            !trade.Indexation.Any() ||
                            trade.Indexation.Any(indexation => !indexation.Document.Any()) ||
                            trade.Indexation.Any(
                                indexation =>
                                    indexation.Document.Any(
                                        document => documentStatus.Contains(document.DocumentStatusId))));
            }
            else
            {
                query =
                    query.Where(
                        trade => trade.Indexation.Any(
                            indexation =>
                                indexation.Document.Any(
                                    document => documentStatus.Contains(document.DocumentStatusId))));
            }
        }

        return query;
    }

Answer 1

You could make an interface and make each one of the filters an object. I've done this when I knew the where clauses could grow with time and/or I needed to unit test each where clause to know the filters where correct.

Something like

public interface ISearchFilter
{
   IQueryable<T>  Apply<T>(IQueryable<T> query, SearchCriteria searchCriteria);
}

Then create classes as

/// assume entity is name of your iqueryableclass
public EventReferenceFilter : ISearchFilter<Entity>
{
   public IQueryable<Entity>  Apply<T>(IQueryable<Entity> query, SearchCriteria searchCriteria)
   {
       if (searchCriteria == null)
       {
           return query;
       }
       return query.Search(x = > x.EventReference, searchCriteria.EventReference);
   }
}

I've used MEF to export all these classes so at run time I can import many and get run though them.

foreach (var filter in filters) // filters would have been populated by MEF
{
     query = filters(query, searchCriteria);
}

Answer 2

Sometimes if I don't need anything OO (Object-Oriented) or it's a part of a bad code anyway I usually refactor something like this into that:

The simplest or maybe quick&dirty way to group all the ifs into some structure that is easier to maintain is to create an array of Funcs that take the search-criteria and the current query as a parameter and return an updated query if the search-criteria is met or an unchanged query if not.

Each element of the array is such an anonymous function evaluating only a single condition and updating the query if necessary.

var queries = new Func<SearchCriteria, IQueryable<T>, IQueryable<T>>[]
{
    (sc, q) => sc.EventReference != null ? q.Search(x = > x.EventReference, sc.EventReference) : q,
    (sc, q) => !sc.PendingEvent ? q.Where(x = > x.EventStatusId != EventStatus.Pending) : q,
    ...
};

Finally you run all functions in a loop and pass the required parameters.

foreach(var queryFunc in queries)
{
    query = queryFunc(searchCriteria, query);
}

For more information see:

• Func Delegate
• foreach, in (C# Reference)
• var (C# Reference)
• ?: Operator (C# Reference)
• Lambda Expressions (C# Programming Guide)
• Arrays (C# Programming Guide)
• Semicolons in C#

Answer 3

SearchCriteria is a parameter and thus should be camelCase.

---

Why assign dteToday when it is only used in two cases? And it's a bad name anyway.

---

I'm a bit baffled by the casting of IDs to classes, e.g. (Remitter) SearchCriteria.RemitterId or (AssetClass) SearchCriteria.AssetClassId). That feels... odd. And counter-intuitive.

---

To me this is bad design:

    if (SearchCriteria.PendingTrade == false)
    {
        query = query.Where(x => x.TradeStatusId != TradeStatus.Pending);
    }
    if (SearchCriteria.VerifiedTrade == false)
    {
        query = query.Where(x => x.TradeStatusId != TradeStatus.Verified);
    }
    if (SearchCriteria.CanceledTrade == false)
    {
        query = query.Where(x => x.TradeStatusId != TradeStatus.Canceled);
    }

I don't see how more than one can be true at the same time, so it should be one property.

Ditto:

    if (SearchCriteria.PendingEvent == false)
    {
        query = query.Where(trade => trade.Event.All(evt => evt.EventStatusId != EventStatus.Pending));
    }
    if (SearchCriteria.VerifiedEvent == false)
    {
        query = query.Where(trade => trade.Event.All(evt => evt.EventStatusId != EventStatus.Verified));
    }
    if (SearchCriteria.CanceledEvent == false)
    {
        query = query.Where(trade => trade.Event.All(evt => evt.EventStatusId != EventStatus.Canceled));
    }

---

This is another design flaw:

    if (SearchCriteria.IsEarlyMatured == false)
    {
        query = query.Where(x => x.IsEarlyMatured != true);
    }

Considering the default value of a boolean is false, you are in danger of unexpectedly adding a query filter. IMHO you should use only nullable booleans in a filter like this. To me the default behavior when sending an "unconfigured" filter to this method -- e.g. new SearchCriteria() without setting any property -- is that a query is constructed without a single WHERE clause.

---

Part of the 50+ lines of shenanigans with documentStatus (bad name since this is a List<DocumentStatus?>) should be moved to a separate method; perhaps even a separate class.

---

In the end you've given us not enough to work with, and I feel your code has far worse problems than 200 lines of ifs. That long list is likely a result of bad design elsewhere, and moving the logic of each of those ifs to a separate class as proposed by @CharlesNRice is merely painting over a bad spot. That approach is still a valid one, but you need to fix the issues in SearchCriteria first.