Rules engine dynamic type casting

Question

I have a basic switch statement that executes some code on expectedValue and givenValue. The code to execute is guided by the variable actionToApply. This leaves one more variable that is fieldTypeName. This essentially guides the code in to casting the values to the type defined by fieldTypeName - if needed.

I was hoping to have the switch block reviewed/refactored - if possible have the casting refactored away into another method that handles casting dynamically on the fly based on fieldTypeName. Then it can cast expectedValue and givenValue to the correct types making the switch statement code less repetitive and have less duplication.

At compile time we have the values as strings and we have the type to CastTo as a string too. I am open to using the dynamic keyword here if someone is brave enough.

Naturally as more types are added to the DB the below switch statement is going to grow and break a few SOLID principles along the way.

private bool Run(string actionToApply, string fieldTypeName, string expectedValue, string givenValue)
{
    switch (actionToApply)
    {
        case "HasValue":
            return !string.IsNullOrWhiteSpace(givenValue) && givenValue != expectedValue;

        case "EqualTo":
            return givenValue == expectedValue;

        case "NotEqualTo":
            return givenValue != expectedValue;

        case "Greater Than":
            if (fieldTypeName == "int")
            {
                var one = Convert.ToInt32(givenValue);
                var two = Convert.ToInt32(expectedValue);
                return one > two;
            }
            if (fieldTypeName == "double")
            {
                var one = Convert.ToDouble(givenValue);
                var two = Convert.ToDouble(givenValue); 
                return one > two;
            }
            return false;
        case "Or":
            var orArrayValues = expectedValue.Split(new[] { SplitOperatorForOr }, StringSplitOptions.None);
            return orArrayValues.Any(x => x == givenValue);
        case "Less Than":
            if (fieldTypeName == "int")
            {
                var one = Convert.ToInt32(givenValue);
                var two = Convert.ToInt32(expectedValue);
                return one < two;
            }
            if (fieldTypeName == "double")
            {
                var one = Convert.ToDouble(givenValue);
                var two = Convert.ToDouble(givenValue);
                return one < two;
            }
            return false;

        case "Range":
            var given = Convert.ToInt32(givenValue);
            var rangeArrayValues = expectedValue.Split(new[] { SplitOperatorForRange }, StringSplitOptions.None);
            if (fieldTypeName == "int")
            {
                var rangeA = (int)Convert.ChangeType(rangeArrayValues[0], typeof(int));
                var rangeB = (int)Convert.ChangeType(rangeArrayValues[1], typeof(int));
                return given > rangeA && given < rangeB;
            }
            if (fieldTypeName == "double")
            {
                var rangeA = (double)Convert.ChangeType(rangeArrayValues[0], typeof(double));
                var rangeB = (double)Convert.ChangeType(rangeArrayValues[1], typeof(double));
                return given > rangeA && given < rangeB;

            }
            return false;
    }
    return false;
}

Answer 1

In order to make your switch testable and extendable you should split it into multiple classes. One class for each operation that could look like this interface:

interface IOperation
{
    bool Evaluate(string actionToApply, string fieldTypeName, string expectedValue, string givenValue);
}

As the actionToApply check would repeat for all operations you can implement it in an abstract class already:

abstract class Operation
{
    public bool Evaluate(string actionToApply, string fieldTypeName, string expectedValue, string givenValue)
    {
        return 
            string.Equals(actionToApply, GetType().Name, StringComparison.OrdinalIgnoreCase) && 
            EvaluateCore(fieldTypeName, expectedValue, givenValue);
    }

    protected abstract bool EvaluateCore(string fieldTypeName, string expectedValue, string givenValue);
}

Other operations would just need to implement one method doing exactly one thing:

class HasValue : Operation
{ 
    protected override bool EvaluateCore(string fieldTypeName, string expectedValue, string givenValue)
    {
        return !string.IsNullOrWhiteSpace(givenValue) && givenValue != expectedValue;
    }
}

Later you can put them all in a collection:

var operations = new IOperation[] { new HasValue(), new EqualTo() };

and get the result with linq very easily:

var result = operations.Any(o => o.Evaluate(a, b, c, d));

Answer 2

Just a quick shot at case "Greater Than":

case "Greater Than":
    if (fieldTypeName == "int")
    {
        var one = Convert.ToInt32(givenValue);
        var two = Convert.ToInt32(expectedValue);
        return one > two;
    }
    if (fieldTypeName == "double")
    {
        var one = Convert.ToDouble(givenValue);
        var two = Convert.ToDouble(givenValue); 
        return one > two;
    }
    return false;  

if fieldTypeName is double it is always returning false because copy&pasta should be done carefully. This applies to Less Than as well.

Answer 3

You didn't give much context to the code. But a few observations come to mind:

• Run and actionToApply sound much too active for the code that is being run. It seems like Evaluate and relationship would be more appropriate.
• Why are the cases inconsistently named? If you have "EqualTo", then I would expect "GreaterThan", not "Greater Than".
• Is string matching appropriate for determining the equality of numeric values? Is "0.10" equal to ".1"? Considering that it's possible for givenValue to be empty, I would assume that these values are arbitrary free-form strings.

Answer 4

General notes:

• The code is very 'stringly typed'. That makes it more difficult to understand and more error-prone. A string can have many possible values, so it's impossible to tell which actions and types this system supports just by looking at this method's interface - you'll have to investigate the code. Use enums instead - they're more self-documenting, faster to compare and less error-prone (no case sensitivity issues or typos).
• There's a lot of string comparisons and parsing mixed with actual evaluation code. I'd separate these two: have parsing code that produces Expression or AST objects from an input string, so the evaluation code can focus on evaluating these expressions.
• If this switch statement is getting out of hand, it may be useful to create a method for each action, and just call those methods from your switch statement. Alternately, if you have an AST, you could let nodes evaluate themself, given some context, or use a visitor approach.
• Error handling is inconsistent: non-numeric inputs for numeric expressions throw low-level exceptions, while passing invalid action or type strings simply produce false. You may want to distinguish between invalid expressions (syntax errors) and invalid values (runtime errors) and create specific exceptions for them. Documenting which exceptions can be thrown, and when, isn't a bad idea either.
• Type handling is also inconsistent: equality checking happens string-wise (completely ignoring the given type), while greater-than and less-than is numeric. You'll want to spend some time to come up with consistent and non-surprising behavior - and be sure to document it for others.
• Run's argument order seems odd: the 'expected' value comes before the 'given' value, but expressions use 'given' as their first argument.
• As far as I know, the terms Evaluate and operator are more commonly used in situations like this than Run and action.

Action-specific:

• Is 'HasValue' meant to be an equality check between two values, or an 'is-empty' check for a single value? I assume it's the latter, but why does it take two inputs? I'm really not sure what this is supposed to do.
• When I read 'Or', I'd expect a short-circuiting boolean operation. The implementation looks more like SQL's IN operator though, so that's probably a better name.
• Likewise, for 'Range' I'd expect something that returns a list of numbers. 'Between' seems like a more suitable name for what it does. You may also want to document whether the check includes or excludes the start and end of the range, and whether it expects a start and end value or a start and count value.

Answer 5

Easy Simplification:

Integers convert up to doubles just fine. Since you aren't returning the number directly you can safely drop the integer case for LessThan, GreaterThan, and Range and always convert the number to a double. You'll get the same result.

case "GreaterThan":
    if (fieldTypeName == "int" || fieldTypeName == "double")
    {
        var one = Convert.ToDouble(givenValue);
        var two = Convert.ToDouble(expectedValue);
        return one > two;
    }
    goto default;

Answer 6

My refactored code:

public abstract class Operation // Could be an interface?
{
    public abstract bool IsMatch(string operationType);

    public abstract bool Evaluate(string expectedValue, string givenValue);
}

public class EqualTo : Operation
{
    public override bool IsMatch(string operationType)
    {
        return operationType == typeof(EqualTo).Name;
    }

    public override bool Evaluate(string expectedValue, string givenValue)
    {
        return expectedValue == givenValue;
    }
}

public class GreaterThan : Operation
{
    public override bool IsMatch(string operationType)
    {
        return operationType == typeof(GreaterThan).Name;
    }

    public override bool Evaluate(string expectedValue, string givenValue)
    {
        // can make assumptions based on the data provided and schema design from DB.
        var given = Convert.ToDouble(givenValue);
        var expected = Convert.ToDouble(expectedValue);
        return given > expected;
    }
}

public interface IOperationsEngine
{
    IEnumerable<Operation> Operations { get; }
    bool Evaluate(string @operator, string expectedValue, string givenValue);
}

public class OperationsEngine : IOperationsEngine
{
    public IEnumerable<Operation> Operations { get; } = new List<Operation>
    {
        new HasValue(),
        new EqualTo(),
        new NotEqualTo(),
        new GreaterThan(),
        new LessThan(),
        new Or(),
        new Range()
    };

    public bool Evaluate(string @operator, string expectedValue, string givenValue)
    {
        var op = Operations
            .SingleOrDefault(x => x.IsMatch(@operator))
            ?.Evaluate(expectedValue, givenValue);

        if (op == null)
        {
            throw new ArgumentException($"Unsupported operator: {nameof(@operator)} provided.");
        }
        return op.Value;
    }
}

USAGE:

private bool EvaluateRule(string @operator, string expectedValue, string givenValue)
{
    _logger.Debug($"ACTION:   {@operator}");
    _logger.Debug($"GIVEN:    {SanitizeForLogging(givenValue)}");
    _logger.Debug($"EXPECTED: {SanitizeForLogging(expectedValue)}");

    return _engine.Evaluate(@operator, expectedValue, givenValue);
}

So i have tested the above proposed changes and my code works exactly the same way as it did before. However it now exhibits more SOLID best practises than it did before.