Applying different filters within loops

Question

What I have is an outer loop which loops through a list of strings, followed by another loop which loops through a list of filters; and if the current column is the current filter in the list of filters, it then applies this filter.

I'm just trying to tidy up a messy function but can't see anyway of doing so.

public static bool PassRules(List<string>columnsList, List<Customfilters> customfilters )
{
    var i = 0;
    foreach (var column in columnsList)
    {
        var curFilter = new Customfilters();

        foreach (var customfilterse in customfilters.Where(customfilterse => customfilterse.Field.Equals(Filed[i])))
        {
            curFilter = customfilterse;
        }

        if ( !string.IsNullOrEmpty(curFilter.Filter))
        {
            int n;
            var isNumeric = int.TryParse(column, out n);
            if (isNumeric)
            {
                if (!Compare(curFilter.Expression, Convert.ToDouble(column), Convert.ToDouble(curFilter.Filter)))
                {
                    return false;
                }
            }
            else
            {
                if (curFilter.Expression.Equals("IN") || curFilter.Expression.Equals("NOT IN"))
                {
                    var toList = curFilter.Filter.Split(Convert.ToChar(","));

                    if (!CompairInNotIn(toList.ToList(), curFilter.Expression, column))
                    {
                        return false;
                    }
                }
                else
                {
                    if (!Compare(curFilter.Expression, column, curFilter.Filter))
                    {
                        return false;
                    }
                }
            }
        }
        i++;
    }
    return true;
}

public static bool Compare<T>(string op, T left, T right) where T : IComparable<T>
{
    switch (op)
    {
        case "<": return left.CompareTo(right) < 0;
        case ">": return left.CompareTo(right) > 0;
        case "<=": return left.CompareTo(right) <= 0;
        case ">=": return left.CompareTo(right) >= 0;
        case "==": return left.Equals(right);
        case "!=": return !left.Equals(right);
        default: throw new ArgumentException("Invalid comparison operator: {0}", op);
    }
}

public static bool CompairInNotIn(List<string> list, string op, string column)
{
    if (op.Equals("IN"))
    {
        return list.Any(column.Contains);
    }
    if (op.Equals("NOT IN"))
    {
        return !list.Any(column.Contains);
    }
    return true;
}

Just to explain a few fields:

var tmpFilter1 = new Customfilters { Field = "IMA01", Expression = "==", Filter = "HP" };

Field[i] is a dictionary of <int,string> and basically returns the ID of the field name.

Answer 1

First of all, some of the naming is a bit odd. Watch out for the following:

• Unnecessary shortenings. curFilter should be currentFilter
• Inconsistent casing. By convention, method parameters and variables local are camelCased, and classes are PascalCased. So customfilters (the parameter) should be customFilters and Customfilters (the class) should be CustomFilters. Except for:
• Singular vs plural. A Customfilters is actually just a single filter. So this should be CustomFilter
• Spelling. At one point you write Compair instead of Compare. Likewise Filed instead of Field I'm also not sure what customfilterse means.

All of this makes reading the code just that bit harder than it should be.

On to the main point: To me, this looks like it's screaming out for the behaviour to be moved out of the current method to the filter class. Classes, after all, exist as containers of related data and functionality, and what's more appropriate functionality for a filter than to do the actual filtering?

So I would suggest that you create an interface like:

public interface IFilter
{
    bool Match(string expression, string value)
}

Here, the 'expression' you pass in would be the filter expression, and 'value' would be the actual column value. A simple implementation might look like:

public class GreaterThanFilter : IFilter
{ 
    bool Match(string expression, string value)
    {
         var expressionNumber = Convert.ToDouble(expression);
         var valueNumber = Convert.ToDouble(value);
         return valueNumber > expressionNumber;
    }
}

Repeat this for the rest of them and you'll probably find you'll have some repeated behaviour which you can pull out into one or more base classes.

There's definitely additional rejigging that can be done after that. For example, consider should the fields you currently have on your Customfilters class be on the same IFilter interface, or elsewhere? But this would be a very good start, which would allow you to massively cut down on the logic in that method.

Answer 2

Can't you change:

if ( !string.IsNullOrEmpty(curFilter.Filter)) {
  ..Lots of code..
}

to

if ( string.IsNullOrEmpty(curFilter.Filter)) {
  i++;
  continue;
}

? This saves you one indent. (The else clause becomes redundant)

Also I would most likely swap around the if statements for isNumeric and the IN and NOT IN (and their corresponding contents). If only because the two function calls:

if (!Compare(curFilter.Expression, Convert.ToDouble(column), Convert.ToDouble(curFilter.Filter))) {
  return false;
}

and

if (!Compare(curFilter.Expression, column, curFilter.Filter)) {
  return false;
}

look very similar, so imo it's better to group them together. (Though this is mere aesthetics)

Other than that I'm looking at some more fundamental changes if you really want to tidy up the structure. But that goes beyond the scope of this post. (I'm at work at the moment, I know, bad excuse. See what other people have to say.)