4
\$\begingroup\$

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;
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Oct 4 '16 at 11:24
6
\$\begingroup\$

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);
}
\$\endgroup\$
6
\$\begingroup\$

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:

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

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ It's out of the question that the overall design of the application seems to be really bad... but on the other side sometimes you can't do anything about it if it's some acient code so improving small parts is better then nothing ;-) \$\endgroup\$ – t3chb0t Oct 4 '16 at 12:03
  • \$\begingroup\$ @BDcotWEB I just read your answer. the points you mentioned are relevants, but for me I was expecting a clean solution as the solutions proposed above. but I take your ideas, they are not bad. \$\endgroup\$ – Coding4Fun Oct 4 '16 at 15:49

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.