# Refactor c# comparision/filter logic

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  ;
}


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.

• Yep I agree in moving complex boolean logic to either a method to to seperate variables with good names – dreza May 10 '12 at 8:54
• >>While all I did was move stuff around -> This is good enough. It is indeed clean code, much much cleaner than original one. – Tilak May 10 '12 at 9:02