Refactor a selection validator

Question

I have a data table and have to validate every field in it. I have refactor this code to this below, but the complexity is 15(!!) Should I make something like dictionary with type as Key and Func as Value?

private bool CheckField(DataRow dataRow, ValidationField validationField)
 {
 bool result = false;
 if (validationField.Requiered)
 {
 if (validationField.Type == typeof (int))
 {
 result = this.CheckIntegerAndNotNull(dataRow[validationField.Name].ToString());
 }
 else if (validationField.Type == typeof (DateTime))
 {
 result = this.CheckDateTimeAndNotNull(dataRow[validationField.Name].ToString());
 }
 }
 else
 {
 if (validationField.Type == typeof (int))
 {
 result = this.CheckIntegerOrNull(dataRow[validationField.Name].ToString());
 }
 else if (validationField.Type == typeof(DateTime))
 {
 result = this.CheckDateTimeOrNull(dataRow[validationField.Name].ToString());
 }
 else if (validationField.Type == typeof(string))
 {
 result = this.CheckStringOrNull(dataRow[validationField.Name].ToString(),
 validationField.MaxLength.Value);
 }
 else if (validationField.Type == typeof(decimal))
 {
 result = this.CheckDecimalOrNull(dataRow[validationField.Name].ToString());
 }
 }
 return result;
 }
 public class ValidationField
 {
 public Type Type { get; set; }
 public string Name { get; set; }
 public bool Requiered { get; set; }
 public int? MaxLength { get; set; }
 }

Answer 1

The first simplification is for the Required check. All you need to do here is check that the value is not null, you don't care about the type. Then you can do the type-specific check later. So remove the outermost if...else and simply have:

if (validationField.Requiered && DBNull.Value.Equals(validationField.Name]))
 return false;

The next section is a bit more difficult. However, one approach would be something like:

var dataString = dataRow[validationField.Name].ToString();
return dataString == Convert.ChangeType(dataString, validationField.Type).ToString();

What we're doing here is using .NET's Convert.ChangeType method to change from the string to your desired primative type. Then, if the string representation of the converted item exactly matches the original string, we say that the string must exactly represent the converted item, meaning it is of the correct data type. You may want to also pass in an IFormatProvider for additional safety.

Putting those two code blocks together would pretty much give you your full method. You may need one additional check for the max length of a string though.

---

Other than the above alternatives, your code mostly looks good. One thing I'd do is scrap the result variable, and return directly from inside your if statements. The this. prefixes are also unnecessary for PascalCased method names.

---

As a more general design note, it might be preferable to do this validation at a different point in your process. For example, DataColumns can be set to require a particular Type, and can also be told whether or not to accept null values using the AllowDBNull property, meaning the table will largely do this validation for you.

Answer 2

Just a small note about this:

public bool Requiered { get; set; }

I can't tell if it's supposed to be Required or Requeried. Either way, it's a typo that needs to be corrected.

Answer 3

the if statements look really messy to me, I would much rather put this into a Switch and use 2 ternary operator statements so that I am only performing if calculations when I absolutely need to.

using the switch I was also able to get rid of the boolean variable and the assignments to that variable, I would much rather just return the result of the "checks"

Here is what I came up with.

private bool CheckField(DataRow dataRow, ValidationField validationField)
{
 switch validationField.Type
 {
 case typeof(int):
 return validationField.Requiered 
 ? this.CheckIntegerAndNotNull(dataRow[validationField.Name].ToString())
 : this.CheckDateTimeOrNull(dataRow[validationField.Name].ToString());
 break;
 case typeof(DateTime):
 return validationField.Requiered 
 ? this.CheckDateTimeAndNotNull(dataRow[validationField.Name].ToString()) 
 : this.CheckDateTimeOrNull(dataRow[validationField.Name].ToString());
 break;
 case typeof(string):
 return this.CheckStringOrNull(dataRow[validationField.Name].ToString(),
 validationField.MaxLength.Value);
 break;
 case typeof(decimal):
 return this.CheckDecimalOrNull(dataRow[validationField.Name].ToString());
 break;
 default :
 return false
 break;
 }
}

Another thought that I had after reviewing this code, I think that you should rename your checks to something a little more specific, I almost blew right past the fact that they should return a boolean result, at least I assume that they should.