Refactor c# comparision/filter logic

Question

I have to write a comparison logic.

Following is the code to refactor (as it is looking bad).

  // 1. item has filter criteria
  // 2. return true if current matches filter criteria
  // 3. Criteria -> 3 fields should be matched ( Security, OrderType, CounterType)
  // 4. Criteria -> User can specify All (*), If any matching field is All, (NONE for enum), only that field should not be matched, other should be matched
  private static bool CompareOrderItem(Item item, Item current)
{
    String all="All";
    bool flag = true;

    if (!( String.Equals(item.Security, all) ||String.Equals(current.Security, item.Security)))
        flag = false;

    if (!( item.OrderType == OrderTypeEnum.None ||Enum.Equals(current.OrderType, item.OrderType)))
        flag = false;


     if (!( String.Equals(item.Counterparty, all) || String.Equals(current.Counterparty, item.Counterparty)))
        flag = false;


    return flag  ;
}

Answer 1

This is a subjective opinion, but I find it useful to extract methods for the three types of matches you are checking in your code.

I refactored your code and extracted the methods:

• bool OrderPartyMatches(Item item, Item current)
• bool OrderTypeMatches(Item item, Item current)
• bool SecurityMatches(Item item, Item current)

While all I did was move stuff around, I think this version gains in readability, because I find

OrderPartyMatches(item, current)

to reveal its intent more clearly than

( String.Equals(item.Security, all) ||String.Equals(current.Security, item.Security)

which is now encapsulated in this named method.

The final code I ended up with is:

const string all = "All";

private static bool CompareOrderItem(Item item, Item current)
{
    return
        SecurityMatches(item, current) &&
        OrderTypeMatches(item, current) &&
        OrderPartyMatches(item, current);
}

private static bool SecurityMatches(Item item, Item current)
{
    return 
        String.Equals(item.Security, all) || 
        String.Equals(current.Security, item.Security);
}

private static bool OrderTypeMatches(Item item, Item current)
{
    return 
        item.OrderType == OrderTypeEnum.None || 
        Equals(current.OrderType, item.OrderType);
}

private static bool OrderPartyMatches(Item item, Item current)
{
    return 
        String.Equals(item.Counterparty, all) || 
        String.Equals(current.Counterparty, item.Counterparty);
}

Please note how the need for comments disappears, as the code now explains itself progressively.

Some people may only be interested to see that three fields are matched (and thus only look at the first method). Others may also be interested to see how the matching is done, and the explanation is just a few lines away.