Refactor this C# code to a more elegant version

Question

I am trying to refactor this code into a more elegant version. Can anyone please help.

The issue is where to as sign the first evaluation result for comparision later on?
And I want to eliminate the use of if/switch if possible
Should I remove Operator class and split Eval into And and Or class, but wouldn't be too much differnt I think

public interface IEval<T>
{
    Func<T, bool> Expression { get; }
    Operator Operator { get; }
    string Key { get; }
}

public static bool Validate<T>(this T o, IList<IEval<T>> conditions)
{
    var returnResult = true;
    var counter = 0;

    foreach (var condition in conditions)
    {
        var tempResult = condition.Expression(o);

        if (counter == 0) //don't like this
        {
            returnResult = tempResult;
            counter++;
        }
        else
        {
            switch (condition.Operator) //don't like this
            {
                case Operator.And:
                    returnResult &= tempResult;
                    break;
                case Operator.Or:
                    returnResult |= tempResult;
                    break;
                default:
                    throw new NotImplementedException();
            }
        }
    }

    return returnResult;
}

Thanks!

Code Updated:

public interface IEval<T>
{
    Func<T, bool> Expression { get; }
    bool Eval(bool against, T t);
}

public class AndEval<T> : IEval<T>
{
    public Func<T, bool> Expression { get; private set; }

    public AndEval(Func<T, bool> expression)
    {
        Expression = expression;
    }

    public bool Eval(bool against, T t)
    {
        return Expression.Invoke(t) & against;
    }
}

public class OrEval<T> : IEval<T>
{
    public Func<T, bool> Expression { get; private set; }

    public OrEval(Func<T, bool> expression)
    {
        Expression = expression;
    }

    public bool Eval(bool against, T t)
    {
        return Expression.Invoke(t) | against;
    }
}

public static class EvalExtensions
{
    public static bool Validate<T>(this T t, IList<IEval<T>> conditions)
    {
        var accumulator = conditions.First().Expression(t);

        foreach (var condition in conditions.Skip(1))
        {
            accumulator = condition.Eval(accumulator, t);
        }

        return accumulator;
    }
}

Is IEval a standard .NET interface? I can't find reference to it anywhere. — Samir Talwar
– Samir Talwar, Commented Dec 22, 2009 at 23:17
This seems wrong on many levels, not least that your AND and OR operators have equal precedence. That's probably not what people writing the code expect. Shouldn't you be using an tree instead of a list? — Mark Byers
– Mark Byers, Commented Dec 22, 2009 at 23:31
I understand your concern but single level comparison was the requirement. Also wouldn't hierarchical comparison be much more harder to implement? — Jeff
– Jeff, Commented Dec 22, 2009 at 23:36
Actually I will refacotr this code into a tree (single node) evaluation. — Jeff
– Jeff, Commented Dec 22, 2009 at 23:50

Mkkt Bkkt · Accepted Answer · 2009-12-22 23:30:24Z

3

Try this (assuming that conditions is not empty)

var accumulator = conditions.First().Expression(0);

foreach (var condition in conditions.Skip(1))
{
    accumulator = condition.Operation.Evaluate(
        condition.Expression(0), accumulator);
}

class AddOperation : Operation
{
    public override int Evaluate(int a, int b)
    {
        return a & b;
    }
}

You get the idea. You can make it even more compact by defining a method in condition that makes it run Expression() on itself and pass the result as the first argument to Evaluate:

condition.Evaluate(accumulator);

class Condition
{
    public int Evaluate(int argument)
    {
        return Operation.Evaluate(Expression(0), argument);
    }
}

(also an unrelated advice: never ever call a variable tempSomething, it is bad karma and creates the impression that you don't know exactly the role of that particular variable for the reader)

edited Dec 22, 2009 at 23:30

answered Dec 22, 2009 at 23:22

Mkkt Bkkt

122k40 gold badges154 silver badges177 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Jeff Over a year ago

i think your solution so far is the best. thanks. thanks also for the adivce

Eric J. · Accepted Answer · 2009-12-22 23:22:58Z

2

One general pattern for eliminating if/switch is to place the logic behind the if in the class you are operating on. I don't know enough about your domain to judge whether that will work here.

To apply that pattern, IEval would be expanded with another method, e.g.

IEval<T>.PerformOperation(T tempResult)

Each concrete implementation of IEval would then implement PerformOperation based on the specific operation it models, rather than using an Enum to indicate the type of operation.

(not sure if tempResult is of type T based on your code).

Then instead of the switch, write

returnResult = condition.PerformOperation(tempResult);

answered Dec 22, 2009 at 23:22

Eric J.

151k65 gold badges353 silver badges563 bronze badges

2 Comments

Jeff Over a year ago

but u still end up having the same problem that you still need to do and/or evaluttion

Eric J. Over a year ago

No you don't. My answer is basically the same as DrJokepu's, but he gave a more complete answer. Have a look at his posting for a clarification.

Martin Jonáš · Accepted Answer · 2009-12-22 23:44:18Z

2

I would go with LINQ methods. Like -

public static bool Validate<T>(this T o, IList<IEval<T>> conditions)
{
    return conditions
        .Skip(1)
        .Aggregate(
            conditions.First().Expression(o),
            (a, b) => b.Operator == Operators.Or ? (a || b.Expression(o)) : (a && b.Expression(o))
        );
}

Or if you don't like ternary operator or need more extensible and nicer approach, you can use Dictionary to store and lookup functions associated with operators.

public static bool Validate<T>(this T o, IList<IEval<T>> conditions)
{
    return conditions
        .Skip(1)
        .Aggregate(
            conditions.First().Expression(o),
            (a, b) => operators[b.Operator](a, b.Expression(o))
        );
}

public static Dictionary<Operator, Func<bool, bool, bool>> operators = new Dictionary<Operator, Func<bool, bool, bool>>()
{
    {Operator.And, (a, b) => a && b},
    {Operator.Or, (a, b) => a || b}
}

answered Dec 22, 2009 at 23:44

Martin Jonáš

2,31715 silver badges12 bronze badges

1 Comment

Jeff Over a year ago

Thanks for the wonderful solution, but I think I prefer to encapsulate and/or logic into separate classes as seen in DrJokepu’s answer. But your one is equally great.

ChrisLively · Accepted Answer · 2009-12-22 23:22:16Z

1

The only thing I can think of is:

Have an if statement with checks that you have at least 2 conditions.

Then, instead of a foreach, use a regular for statement with a counter that starts at the second condition.

If you have zero conditions, return true. depending on your other business logic.

If you have one condition, then take the value.

Regardless, I believe the switch statement for the operation to perform is going to be necessary... Unless you change the code to execute some type of script which is your actual op to perform. Which I think is worse.

answered Dec 22, 2009 at 23:22

ChrisLively

88.3k27 gold badges174 silver badges249 bronze badges

Comments

erikkallen · Accepted Answer · 2009-12-22 23:39:31Z

1

The only thing I don't like is that you have a variable called counter that will always be either 0 or 1. I'd make it a bool isFirst instead. If you want to get rid of the switch, you could replace your IEval interface with

public interface IEval<T>{
    Func<T, bool> Expression { get; }
    Func<bool, bool, bool> Combinator { get; }
    string Key { get; }
}

Your Combine method will then be either

public Func<bool, bool, bool> Combinator {
    get { return (b1, b2) => b1 | b2; }
}

or

public Func<bool, bool, bool> Combinator {
    get { return (b1, b2) => b1 & b2; }
}

depending on the desired operation.

Might be overkill to return a delegate, though, perhaps just add a method bool Combine(bool value1, bool value2)

edited Dec 22, 2009 at 23:39

answered Dec 22, 2009 at 23:23

erikkallen

34.6k14 gold badges90 silver badges121 bronze badges

2 Comments

Jeff Over a year ago

do you mind elborate with how you would get rid of switch? thanks

erikkallen Over a year ago

Sorry, I forgot one bool in the argument list. Elaboration added.

JSBձոգչ · Accepted Answer · 2009-12-22 23:26:35Z

0

The following algorithm exhibits short-circuiting (it stops evaluating once the condition is known to be false). It has the same basic design, but it effectively uses an implicit true && ... at the beginning to make things cleaner.

public static bool Validate<T>(this T o, IList<IEval<T>> conditions)
{
    bool result = true;
    Operator op = Operator.And;
    var conditionIter = conditions.GetEnumerator();

    while (result && conditionIter.MoveNext())
    {
        bool tempResult = conditionIter.Current.Expression(o);
        switch (op)
        {
            case Operator.And:
                result &= tempResult;
                break;
            case Operator.Or:
                result |= tempResult;
                break;
            default:
                throw new NotImplementedException();
        }
        op = condition.Operator;
    }

    return result;
}

answered Dec 22, 2009 at 23:26

JSBձոգչ

41.6k19 gold badges106 silver badges173 bronze badges

1 Comment

Andrew Shepherd Over a year ago

If a subsequent condition is an OR condition, then it would change the result from false to true. You can not confidently stop the loop just because the result is temporarily false.

Collectives™ on Stack Overflow

Refactor this C# code to a more elegant version

6 Answers 6

1 Comment

2 Comments

1 Comment

Comments

2 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

1 Comment

2 Comments

1 Comment

Comments

2 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related