3
\$\begingroup\$

I am currently working on a method, that can search based on a value and an operator.

Currently I check which operator was supplied to the the method in a switch, and do operations accordingly to it.

This is the current code:

switch (enumOperator)
{
    case Operator.Equal:
        tmpResult = tmpResult.Where(a => a.NUMERIC_VALUE == value);
        break;
    case Operator.Less:
        tmpResult = tmpResult.Where(a => a.NUMERIC_VALUE < value);
        break;
    case Operator.LessOrEqual:
        tmpResult = tmpResult.Where(a => a.NUMERIC_VALUE <= value);
        break;
    case Operator.More:
        tmpResult = tmpResult.Where(a => a.NUMERIC_VALUE > value);
        break;
    case Operator.MoreOrEqual:
        tmpResult = tmpResult.Where(a => a.NUMERIC_VALUE >= value);
        break;
    case Operator.NotEqual:
        tmpResult = tmpResult.Where(a => a.NUMERIC_VALUE != value);
        break;
}

Is there a better/more succinct way to represent the previous switch in c#?


Additional info

The previous piece of code is used to search in a database based on the parameter(the field to be searched by), the value and operator.

So the previous code is copied for each field (a => a.X, a => a.Y ...).

As some of the fields are of different type, the "ugly" code is a bit changed and copied again.

Besides of the big switch (which by @unholysampler answer) will be nicer, the code a => a.X == value will be repeated many times(3).

Would it be also possible to save predicates where a => a.X == value, X is yet to be specified?

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! Could you provide a bit more context to this? What does it look like within the class, what is it used for, etc. See How do I ask a good question? \$\endgroup\$ – Phrancis Mar 30 '16 at 14:31
7
\$\begingroup\$

There are a few ways to approach this base on how much you want to minimize the code.

Define the predicates as actual functions and pass them in by name.

case Operator.Equal:
    tmpResult = tmpResult.Where(NumericEqual);
    break;

Use a temporary variable for the argument to Where()

Func<T, Boolean> predicate;
switch (enumOperator)
{
    case Operator.Equal:
        predicate = a => a.NUMERIC_VALUE == value;
        break;
    case Operator.Less:
        predicate = a => a.NUMERIC_VALUE < value;
        break;
    // ...
}
tmpResult = tmpResult.Where(predicate);

This could also be done with a function that encapsulates the switch and hiding it from the main function.

Or, if you feel like the switch statement is causing too much boilerplate, you could make a Dictionary<Operator, Func<T, Boolean>> and insert a function pointer for each operator. Then the code that uses it becomes:

tmpResult = tmpResult.Where(operatorMap[enumOperator]);

It is a trade off. The shorter code requires the use of more memory. Handling the case where enumOperator does not have match (which is not currently being done) would also have to change.


For the case where you will have lots of different properties that all need similar operations, you can have a function that creates the predicate.

Func<T, Boolean> MakePredicateEqual(Func<T, X> getField, X value)
{
    return a => getField(a) == value;
}

However, as things become more generic, more things need to be passed in as arguments and plumbing things together gets more complicated. It needs to be determined when the diminishing returns make it not worth it.

\$\endgroup\$
  • \$\begingroup\$ Would it also be possible to create more flexible predicates, as described in the question? \$\endgroup\$ – Aleksandar Mar 30 '16 at 17:46
  • \$\begingroup\$ @Aleksandar See edit for the more generic version. \$\endgroup\$ – unholysampler Mar 30 '16 at 18:30
2
\$\begingroup\$

Disclaimer

I just want to start by saying I don't recommend doing this and you should go with unholysampler's answer (or your current solution; You're not saving much code there).

I'm posting this possibility to show you what could be and why you shouldn't do it. I was once in a similar place to you and thought it would be great to do something like this, but it was not worth the time or effort (though it was a nice learning experience).

"Solution"

Okay, so you can technically make a function where you build the expression from a property selector, operator (enum), and value you're comparing to.

Downfall #1: Using this method, you'll see that you're just moving the switch-statement you're trying to get rid of. So if your goal is to reduce the number of lines of code this won't help.

NOTE: This was written in a unit test class.

private static Expression<Func<T, bool>> PredicateBuilder<T, TProp>(Expression<Func<T, TProp>> propertySelector, Operator comparison, TProp value)
{
    var args = propertySelector.Parameters;

    Expression body;
    switch (comparison)
    {
        case Operator.Less:
            body = Expression.LessThan(
                propertySelector.Body,
                Expression.Constant(value)
            );
            break;
        // etc. etc.
        default:
            body = Expression.Equal(
                propertySelector.Body,
                Expression.Constant(value)
            );
            break;
    }

    return Expression.Lambda<Func<T, bool>>(body, args);
}

Usage: Unit Test

I used a simple "Tile" class that you can see the properties of in their initializers.

Downfall #2: If you use that PredicateBuilder function on a type that doesn't implement some of the operators it will throw an exception. (e.g. string and '>=', '<', etc).

[TestMethod]
public void PredicateBuilderBasicTest()
{
    // ASSEMBLE: Create a list of objects for filtering.
    IEnumerable<Tile> tiles = new List<Tile>()
    {
        new Tile() { Name = "Solid", Type = "Grass", X = 0, Y = 0, IsBroken = false },
        new Tile() { Name = "Liquid", Type = "Water", X = 1, Y = 0, IsBroken = false },
        new Tile() { Name = "Broken", Type = "Grass", X = 0, Y = 1, IsBroken = true },
        new Tile() { Name = "Liquid", Type = "Water", X = 1, Y = 1, IsBroken = false },
        new Tile() { Name = "Solid", Type = "Grass", X = 0, Y = 2, IsBroken = false },
        new Tile() { Name = "Liquid", Type = "Water", X = 2, Y = 0, IsBroken = false },
        new Tile() { Name = "Liquid", Type = "Water", X = 2, Y = 1, IsBroken = false },
        new Tile() { Name = "Solid", Type = "Dirt", X = 1, Y = 2, IsBroken = false },
        new Tile() { Name = "Broken", Type = "Dirt", X = 2, Y = 2, IsBroken = true }
    };
    var comparison = Operator.Less;
    var value = 2;

    // ACT: All tiles where their X is less than 2.
    var expression = PredicateBuilder<Tile, int>(x => x.X, comparison, value);
    var predicate = expression.Compile();

    // ASSERT: Since there are 9 tiles and 3 with X >= 2. Then there should be 6 tiles after the predicate.
    Assert.AreEqual<int>(6, tiles.Count(predicate));
}

As you could see, you can just reduce that switch to something like:

var predicate = PredicateBuilder<T, TProp>(a => a.NUMERIC_VALUE, enumOperator, value).Compile();
tmpResult = tmpResult.Where(predicate);

But, I still don't recommend doing this. So just to re-iterate reasons not to do this:

  • More code (just less in the place you want).
  • Increased Complexity (making it harder to maintain when something goes wrong).
  • Decreased performance (This might not matter than much and probably will be negligible, but still).
  • Increased room for error (when someone does something like PredicateBuildering<T, string>(x => x.SomeString, Operator.Less, "Testing"); and throws an exception because strings don't implement the < operator).

Hopefully this will make sense to you. I hope this information is useful.

\$\endgroup\$

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.