4

I am trying to refactor a switch statement that is like this:

private void Validate(DataTypeEnum dataType, string value, ...)
{
    switch(dataType)
    {
        case DataTypeEnum.Number:
            var typedValue = int.Parse(value);
            //Validation of typedValue
            break;
        case DataTypeEnum.Decimal:
            var typedValue = Decimal.Parse(value);
            //Validation of typedValue
            break;
        case DataTypeEnum.DateTime:
            var typedValue = DateTime.Parse(value);
            //Validation of typedValue
            break;
    }
}

I would like to get rid of the switch statement and somehow replace it with a more object oriented construct. Any suggestions?

My ideal would be something like this:

private void Validate(DataTypeEnum dataType, string value, ...)
{
    Validate(value);
}
private void (Decimal value)
{
    //Validate
}
private void (DateTime value)
{
    //Validate
}

Is there any elegant way to fix this?

  • 3
    This question about [the visitor pattern](http://stackoverflow.com/questions/255214/when-should-i-use-the-visitor-design-pattern) will help you – durron597 Jul 09 '15 at 14:27
  • 1
    durron597: at this point, I do not have a typed object, I simply have a string which I need to parse and some type enum. –  Jul 09 '15 at 14:30
  • 5
    Then why are you bothering with anything else but a `switch` statement? If you want object-oriented, you need bona fide objects. – Robert Harvey Jul 09 '15 at 14:32

7 Answers7

6

Use polymorphism.

Example:

public class DataType
{
    public virtual void Validate()
    {
        Console.WriteLine("Performing base class validation tasks");
    }
}

class Foo : DataType
{
    public override void Validate()
    {
        // Code to validate a foo...
        Console.WriteLine("Validating a foo");
    }
}
class Bar : DataType
{
    public override void Validate()
    {
        // Code to validate a bar...
        Console.WriteLine("Validating a bar");
    }
}


List<DataType> datatypes = new List<DataType>();
datatypes.Add(new Foo());
datatypes.Add(new Barr());

foreach (DataType s in datatypes)
{
    s.Validate();
}
MetaFight
  • 1,295
  • 14
  • 30
kidra.pazzo
  • 195
  • 8
  • What has this to do with data validation ? The question was not about behavior but about data. – Fabjan Jul 09 '15 at 14:19
  • 3
    @Fabjan, oh come on. The original question wasn't *about* validation. It just used that as an example. This answer uses another example. – MetaFight Jul 09 '15 at 14:23
  • It's too wide then because i couldn't get what's being asked. Think about it : private void Validate(DataTypeEnum dataType, object value) doesn't looks like data validation method to you ? – Fabjan Jul 09 '15 at 14:25
  • 1
    I've suggested an edit to align the examples. – MetaFight Jul 09 '15 at 14:27
  • @Fabjan Then just change the method names from `Draw()` to `Validate()`. – Robert Harvey Jul 09 '15 at 14:27
  • 1
    maybe this is the good design but i dont think this is the answer. OP have string as object sent to Validate. but i dont see any use of the object value here. I know you can add to list like that... but you cant give suggestion like this while you dont know what is happening before Validate method. – M.kazem Akhgary Jul 09 '15 at 14:37
  • 1
    OP use enum DataTypeEnum not classes also how it will validate types like int, string, bool and such ? This is good answer only if we work with user classes – Fabjan Jul 09 '15 at 14:38
5

To add to this, while some might consider this not OOP at all, you can use dynamic overloads to handle this:

public bool ValidateAny(dynamic val)
{
  return Validate(val);
}

private bool Validate(string val) { ... }
private bool Validate(int val) { ... }
private bool Validate(decimal val) { ... }

private bool Validate(object val) { ... } // This is the fallback

Basically, this works the same as usual overload resolution, but it's performed at runtime, depending on the runtime type of the val in ValidateAny.

For example:

ValidateAny(3); // Uses the int overload
ValidateAny((object)3); // Uses the int overload as well - dynamic handles the unboxing
ValidateAny(3M); // Uses the decimal overload
ValidateAny(3.ToString()); // Uses the string overload
ValidateAny(3f); // Uses the object overload, since there's no better match

This is incredibly powerful, as long as you only ever need to have different validations for different types. If you also have other considerations, this must at some level go back to if/switch. Even then it can save you a lot of time, though.

Luaan
  • 62,244
  • 7
  • 97
  • 116
  • This is very cool. I can think of a number of other uses for this. – Robert Harvey Jul 09 '15 at 14:41
  • 1
    And it is also slow as hell not to mention that it can potentially lead to run-time and compile-time problems as IS is not working – Fabjan Jul 09 '15 at 14:46
  • @Fabjan I wonder where you got that impression - even using an entirely naïve benchmark, it's not even ten times slower than simple method invocation. That still means that you can invoke it about five or ten millions of times per second! In any case, it's going to be dwarfed by the implied string operations elsewhere. `is` works just fine - I'm not sure what you're trying to warn about. And yes, it can lead to runtime problems - just like deciding based on a dictionary or using class polymorphism. – Luaan Jul 09 '15 at 14:52
  • Dynamic value type is working with DLR and it takes A LOT of time for initialization. It's ok for operations like responding to user request from server because some extra 100 ms pass unseen as client is busy with its own tasks to handle anyway but it's bad practice to use dynamics if you can use struct types and be fine – Fabjan Jul 09 '15 at 15:05
  • @Fabjan Just use whatever makes the most sense. You can always rewrite it using a non-dynamic approach if it ever becomes a performance problem. In most cases, it will not even come close - in fact, I've never seen such a case in my practice so far. You're far overestimating the performance impact, and far underestimating the savings in clarity and programming time. – Luaan Jul 09 '15 at 15:16
1

First of all I'd start with implementing simple interface for validation:

public interface IValidator
{
    bool Validate(object value);
}

Then number validator may look like this:

public class NumberValidator : IValidator
{
    public bool Validate(object value)
    {
        return (int) value > 0;
    }
}

And final step replacing your switch with dictionary:

 var _validators = new Dictionary<DataTypeEnum, IValidator> // injected via DI
 {
     { DataTypeEnum.Number, new NumberValidator() },
     { DataTypeEnum.DateTime, new DateTimeValidator() },
     { DataTypeEnum.String, new StringValidator() }
 };

 ......

 private bool Validate(DataTypeEnum dataType, object value, ...)
 {
     if (_validators.ContainsKey(dataType))
     {
        return _validators[dataType].Validate(value);
     }

     return false;
 }
Andriy Horen
  • 2,861
  • 4
  • 18
  • 38
  • You're missing one key point - there's no `IValidator` in your example, so the code will not compile. C# doesn't have anything like Java's `IGeneric>`. – Luaan Jul 09 '15 at 14:37
  • Good point, updated my answer to have non-generic `IValidator` interface – Andriy Horen Jul 09 '15 at 15:04
  • Thank you, that's a good solution. –  Jul 09 '15 at 15:05
  • @RobinNadeau And of course, you don't even have to use some `IValidator` - a simple `Func` will do. The dictionary would then have elements like `{ DataTypeEnum.Number, v => ((int)v) > 0 }`. In the end, it doesn't mean much of a difference, as long as your interface only has a single method anyway. – Luaan Jul 09 '15 at 15:18
0
    public interface IValidator
    {
        void Validate(object value);
    }

    public class NumberValidator : IValidator
    {
        public void Validate(object value)
        {
            //implementation
        }
    }

    public class DecimalValidator : IValidator
    {
        public void Validate(object value)
        {
            //implementation
        }
    }

    public class DatetimeValidator : IValidator
    {
        public void Validate(object value)
        {
            //implementation
        }
    }

    private void Validate(IValidator validator, object value)
    {
        validator.Validate(value);
    }
MetaFight
  • 1,295
  • 14
  • 30
Dmitri Tsoy
  • 561
  • 6
  • 21
  • so what is `value` for then? – MetaFight Jul 09 '15 at 14:33
  • well, it makes more sense now, but it's still brittle as hell. What's to keep people from passing in `validator` and `value` instances that are incompatible? – MetaFight Jul 09 '15 at 14:40
  • Than you need to take a next step in refectoring your code. Value should become a property of IValidator. IValidator should become a generic type. – Dmitri Tsoy Jul 09 '15 at 14:41
0

Create a Dictionary that has DataTypeEnum as Key and Action as Value. Then Get the Value from Dictionary by Key and Invoke The action.

private readonly Dictionary<DataTypeEnum , Action > Validator = new Dictionary<DataTypeEnum , Action >
        {
            { DataTypeEnum.Number, () => Validate(Convert.ToDouble((string)value)) },
            { DataTypeEnum.Decimal, () => Validate(Convert.ToDecimal((string)value)) },
            { DataTypeEnum.DateTime, () => Validate(Convert.ToDateTime((string)value)) },
            // ...
        }

private void Validate(DataTypeEnum dataType, object value, ...)
{
    Validator[dataType](); // Gets the Action and Invokes it
}

private void Validate (Decimal value)
{
    //Validate
}
private void Validate (DateTime value)
{
    //Validate
}
//...

Here the Action is Void delegate that takes no parameters. It will convert value to suitable format and calls Validate Method.

M.kazem Akhgary
  • 18,645
  • 8
  • 57
  • 118
-5
private void Validate<T>(T value) where T : IComparable
{
    if(value is Number)
    {

    }
    if(value is Decimal)
    {

    }
    if(value is DateTime)
    {

    }
}
maraaaaaaaa
  • 7,749
  • 2
  • 22
  • 37
-6

How about something like this:

private void Validate(object value, ...)
{
    if(Reference.Equals(null, value)
        return;

    if(value is Number)
    {

    }
    else if(value is Decimal)
    {

    }
    else if (value is DateTime)
    {

    }
    else return;
}

If the value passed in is either Number, Decimal or DateTime it will run the appropriate method, other wise it will simply return.

sidjames
  • 117
  • 6
  • the return at the end of your method is redundant – maraaaaaaaa Jul 09 '15 at 14:28
  • true, but makes it read easier to me and doesn't hurt. you could also replace it with an actual method to handle times when the passed value was not one of the intended data types. – sidjames Jul 09 '15 at 14:37
  • 2
    Including unnecessary code makes me (the guy that will read your code after you find another job) wonder why you put it there. That costs me time I could have spent doing something else. – Robert Harvey Jul 09 '15 at 15:06
  • lmao. I code for fun, the chances of you replacing me in my job are a big fat 0. No need to be insulting due to 1 unneeded line that could be seen as a place holder in it's context. – sidjames Jul 09 '15 at 15:16