tags:

views:

290

answers:

6

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;
    }
}
+2  A: 

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)

DrJokepu
i think your solution so far is the best. thanks. thanks also for the adivce
Jeffrey C
well, thanks, I'm glad I could help.
DrJokepu
+1  A: 

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.

Chris Lively
+2  A: 

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);
Eric J.
but u still end up having the same problem that you still need to do and/or evaluttion
Jeffrey C
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.
Eric J.
+1  A: 

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)

erikkallen
do you mind elborate with how you would get rid of switch? thanks
Jeffrey C
Sorry, I forgot one bool in the argument list. Elaboration added.
erikkallen
A: 

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;
}
JSBangs
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.
Andrew Shepherd
+2  A: 

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}
}
Matajon
+1: this is exactly the answer I came up with :)
Juliet
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.
Jeffrey C