Refactor this C# code to a more elegant version

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

Than开发者_开发问答ks!

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

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)

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

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

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.

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)

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