9
\$\begingroup\$

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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't get why those are not just different calls. \$\endgroup\$ – paparazzo Jul 27 '17 at 10:53
  • \$\begingroup\$ I have rolled back the last edit. Please see What should I do when someone answers my question? Do not add an improved version of the code after receiving an answer. Including revised versions of the code makes the question confusing, especially if someone later reviews the newer code. \$\endgroup\$ – t3chb0t Jul 28 '17 at 16:44
3
\$\begingroup\$

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));
\$\endgroup\$
  • \$\begingroup\$ I think that reusing the same return value to mean both 'the result of an operation' and 'is this the operation that we need' is confusing. Running through all operations is also slower than it needs to be. Why not have a separate method that returns the appropriate IOperation for a given action? And why not use methods instead of classes, since they're stateless and only implement a single method? Same idea, just a little more succinct I think. \$\endgroup\$ – Pieter Witvoet Jul 28 '17 at 22:17
  • \$\begingroup\$ @PieterWitvoet because the application is easier to extend with classes and you can easier put them in a configuration file like json (should you want to do this). They are going to be automatically deserialized and a collection would be automatically created. They are also easier to find (as separatate files) and easier to maintain. You can also easier extend them because if you need aditional parameters you can just add a constructor or other properties. \$\endgroup\$ – t3chb0t Jul 29 '17 at 6:46
  • \$\begingroup\$ @PieterWitvoet besides this is just an example, you can add more methods and one can be called Applies to find an operation first and the other would be Evaluate. It doesn't have to be necessarily a single method per class. You build what you need but the bottomline is you should have separate classes for each validation. \$\endgroup\$ – t3chb0t Jul 29 '17 at 6:48
  • \$\begingroup\$ Methods vs. classes depends on how simple or complex you expect a system to become. Methods require less 'ceremony', but they're more difficult to extend. Serialization isn't an issue if there's no state, and configurability can be achieved in both cases by using a dictionary instead of a hard-coded switch statement. If you're going to use classes, you might as well go all the way towards an AST. Parsing (deserialization) code would give you properly typed (and stateful) nodes, and each node could Evaluate itself. \$\endgroup\$ – Pieter Witvoet Jul 29 '17 at 10:45
  • \$\begingroup\$ But perhaps I shouldn't have mentioned methods at all. The main problem I see here is that the result of Evaluate has two meanings. Evaluate shouldn't need an action name argument - the action name should be used at an earlier stage to select the appropriate IOperation. \$\endgroup\$ – Pieter Witvoet Jul 29 '17 at 10:52
10
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Copy&Pasta is the first thing I warn people about when they are learning to code. \$\endgroup\$ – VisualMelon Jul 27 '17 at 8:53
  • 2
    \$\begingroup\$ @IbrarMumtaz I believe the bigger issue is that you aren't using expectedValue, but instead using givenValue twice. \$\endgroup\$ – Brian J Jul 27 '17 at 15:52
  • \$\begingroup\$ Hi all points raised have been addressed and incorporated - TY. \$\endgroup\$ – IbrarMumtaz Jul 28 '17 at 9:17
6
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ 1 and 2 Noted. It's a WIP project but they can be corrected easily. They are just values stored in the DB. All stored as string and in some cases as seen in the example casting is needed depending on what the action to apply is. So far the needs are simple so If i need equality on ''0.20'' then I will cast it first. \$\endgroup\$ – IbrarMumtaz Jul 27 '17 at 10:52
6
\$\begingroup\$

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.
\$\endgroup\$
3
\$\begingroup\$

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;
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Just my two cents. \$\endgroup\$ – IbrarMumtaz Jul 31 '17 at 16:47
  • \$\begingroup\$ This looks very nice ;-) Regarding your question Could be an interface? This could be an interface. Usually when there is no default or common implementation an interface would be more preferable. Later if you see that some code is repeated for every derived class, you can move it to the abstract class and change the base type form the interface to the abstract class and everything would work as before with now less code repetition. \$\endgroup\$ – t3chb0t Jul 31 '17 at 16:49
  • \$\begingroup\$ @t3chb0t - could not agree more! \$\endgroup\$ – IbrarMumtaz Jul 31 '17 at 16:51

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.