C# - exercise - object validation - reflection - generics

Question

Just published (GitHub) a C# example for object Validation, for this purpose I decided to use the .NET Reflection and tried to use Generics. I would really like to improve coding, can you please give me for your feedbacks / reviews / improvements and also new features to implement ?

Here's the code:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Reflection;
namespace Validation
{
 class Data
 {
 public string EMail { get; set; }
 public DateTime DateOfBirth { get; set; }
 public object Age { get; set; }
 public object PurchasedItems { get; set; }
 public object DateLastPurchase { get; set; }
 public Data() {
 this.EMail = "[email protected]";
 this.DateOfBirth = DateTime.Parse("06/06/1981");
 this.Age = 36;
 this.PurchasedItems = "3";
 this.DateLastPurchase= "06/06/2017";
 }
 }
}
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Validation;
using System.Net.Mail;
namespace Validation
{
 class Program
 {
 public static bool coreLogic(object field)
 {
 return true;
 }
 public delegate bool Del(object field);
 static void Main(string[] args)
 {
 Data obj = new Data();
 Validation v_obj = new Validation(obj);
 Dictionary<string, FieldType> y = v_obj.p_validationFields;
 y["EMail"] = FieldType.EMail;
 y["DateOfBirth"] = FieldType.DateTime;
 y["Age"] = FieldType.Integer;
 y["PurchasedItems"] = FieldType.Integer;
 y["DateLastPurchase"] = FieldType.DateTime;
 v_obj.p_hHandlers = new Hashtable();
 v_obj.p_hHandlers.Add(FieldType.EMail, (Del)coreLogic);
 bool b = v_obj.setValidationFields(y).doValidation();
 }
 }
}
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Net.Mail;
using System.Reflection;
namespace Validation
{
 public enum FieldType
 {
 NoValidation,
 EMail,
 DateTime,
 Integer
 }
 public class Validation
 {
 private object p_obj { get; set; }
 public Dictionary<string, FieldType> p_validationFields { get; set; }
 public Hashtable p_hHandlers { get; set; }
 public Validation(object o)
 {
 this.p_obj = o;
 Dictionary<string, FieldType> d = new Dictionary<string, FieldType>();
 foreach (PropertyInfo p in o.GetType().GetRuntimeProperties())
 d.Add(p.Name, FieldType.NoValidation);
 this.p_validationFields = d;
 }
 public Validation setValidationFields(Dictionary<string, FieldType> f)
 {
 this.p_validationFields = f;
 return this;
 }
 public bool doValidation()
 {
 bool isValid = false;
 Dictionary<string, FieldType> d = this.p_validationFields;
 foreach (PropertyInfo p in this.p_obj.GetType().GetProperties())
 {
 FieldType f;
 try
 {
 f = d[p.Name];
 if (d.ContainsKey(p.Name))
 {
 isValid = ValidateField(p.GetValue(this.p_obj, null), f);
 if (!isValid)
 break;
 }
 }
 catch (Exception e) { isValid = false; }
 }
 return isValid;
 }
 private Boolean ValidateField(object field, FieldType type)
 {
 Boolean valid = false;
 switch (type)
 {
 case FieldType.EMail:
 valid = false;
 // default validation for EMail
 try {
 MailAddress m = new MailAddress((string)field);
 valid = true; }
 catch (FormatException) {
 valid = false; }
 // override the default validation for EMail
 Delegate handler = (Delegate)this.p_hHandlers[FieldType.EMail];
 if (handler!=null) valid = (bool)handler.DynamicInvoke(field);
 break;
 case FieldType.DateTime:
 DateTime d; valid = DateTime.TryParse(field.ToString(), out d);
 if (d > DateTime.Now) valid = false;
 break;
 case FieldType.Integer:
 int n; bool b = int.TryParse(field.ToString(), out n); valid = b;
 break;
 case FieldType.NoValidation:
 valid = true;
 break;
 }
 return valid;
 }
 }
}

Look forward being in touch with you :)

• Marco Salerno: Please stop giving one char name to variables
• Gae: thanks but besides that, can you provide feedback about the logic other than naming convention used ? :)
• Igor Soloydenko: @Gae reviewing cryptic code is not fun. Style is important. Analyzing the logic of hard-to-read code requires much much more mental resources than it should. I'm surprised there's a highly detailed & quality answer from Pieter.
• Igor Soloydenko: Here's one constructive idea though. Validation is usually not primitive data-type specific. On contrary, validation is model-driven or model-specific. I.e. different string, DateTime, etc. are validated based on different rules. Even things like Money, Age, and similar may often be validated quite differently depending on context. I personally have never seen [overly] generalized validation approach. Neither I saw field-level validation to ever be a complete solution. Domain models and what they represent should be in the focus, IMO
• Igor Soloydenko: It's good that you're trying to solve problem like that. Trying to [re]invent a wheel is fun, great practice, and sometimes source of really fresh ideas. Nevertheless, it usually leads to picking an existing solution the community has created by now. github.com/JeremySkinner/FluentValidation may be an example of a decent validation framework. (DO NOT look at it as long as you are done with your exercise!) :)

Answer 1

Overall design

• Having to create and configure a separate Validation object for each object that you want to validate is quite cumbersome. A reusable Validation object seems much more useful.
• It may want to indicate which properties are invalid, and which validation rules they invalidate - especially when you want to give feedback to a user. A simple bool result is often not sufficient.
• Validation often depends on context. d > DateTime.Now might make sense for a birth date, but not for a (future) meeting. Hard-coding specific types/rules likely isn't flexible enough - allowing custom validation methods is better.
• You do actually support custom validation, but why only for email types? Either document it, or take away that restriction - the current behavior is quite surprising (and code being surprising is often not a good thing).

Class design

• Don't give properties public setters if you don't have to - don't even make them public if you can get away with it. Allowing other code to mess with the internals of a class can cause subtle problems over time. For example, you're not checking or guarding against p_validationFields being null. The current implementation still works in that case, but that seems more by coincidence than by design.
• Likewise, methods should verify their input. For example, the constructor could check if o is null and throw an ArgumentNullException(nameof(o)) in that case. The same goes for setValidationFields. Try to ensure that your classes are always in a valid state (no pun intended).
• Creating a new throwaway MailAddress object to validate an email string is somewhat odd - I'd create a MailAddress.TryParse or MailAddress.IsValid method instead.
• Don't give a class the same name as its parent namespace.
• Validation.p_hHandlers should be a Dictionary<string, Del> (or Dictionary<string, Func<object, bool>>) instead of a Hashtable. Not only does this prevent incorrect use, it also makes its purpose a little more obvious.
• Data may be an example class, but having properties of type object is rarely a good idea. Age should probably be an int or int?, PurchasedItems a List<T> of some kind, and DateLastPurchase a DateTime.

Code comprehension

• Using clear, meaningful variable names will greatly improve the readability of your code - and that makes it easier to work with. Some names, such as d, p, p_obj, p_hHandlers and Del are very cryptic - fieldValidationTypes, validationTarget, propertyInfo, customValidationRules and ValidationDelegate would be better.
• Try using unambiguous names. field is somewhat confusing: you're validating a value, not a field (which, given the context, might be mistaken for a FieldInfo). Actually, you're working with properties, not fields, and mixing up those terms might also cause some confusion.
• doValidation is perhaps better renamed to IsValid - it makes calling code look a bit more natural. Also, Validator is better than Validation: use nouns for classes and verbs for methods.
• Don't put the body of an if/catch statement on the same line - it makes control flow harder to see.

Style

• In C#, the standard naming convention is to use PascalCase for class and method names, and camelCase for variables. I recommend sticking to that, but whatever you do, at least be consistent.
• The same goes for curly brace placement. Inconsistency hinders readability.
• And the same goes for primitive type names: if you want to use Boolean, use it everywhere, and also use String, Object, Int32 and so on. I would use bool (and string, object, int, ...) instead.
• That p_ prefix seems to be some kind of Hungarian notation, but it's not clear what it stands for. If it means 'property', then it's useless, because a good IDE can tell you that already. If it means 'pointer', then it's wrong.

Other notes

• GetRuntimeProperties() also returns static and private properties. You may want to use GetProperties() instead.
• Declare variables as close to where they are used as possible, in the smallest possible scope, and initialize them immediately (FieldType f;).
• doValidation tries to get an item from p_validationFields (d) before checking whether that key exists. That's doing things in the wrong order. Note that since C# 6, you can write if (dictionary.TryGetValue(key, out ValueType value)) { /* use value */ }, and C# 7 introduces 'discards', for when you don't need to use the out result at all: TryGetValue(key, out _).
• In doValidation, you're breaking out of a loop only to then return isValid. Directly returning false at that point may be more clear. The same goes for ValidateField: rather than setting a variable and then returning that, each case might as well bail out immediately if there's nothing else left to do.
• Initializing p_validationFields with NoValidation for each property isn't useful: the absence of a validation type should be sufficient.
• Rather than passing an object to Validation, consider passing a Type - or perhaps make it a generic class (Validation<T>), and use typeof(T). doValidation can then accept the object to be validated (which can be strongly typed if Validation is generic), which makes your Validation class reusable.
• doValidation doesn't need to fetch all properties, it only needs to process properties that actually have a validation type. If you use PropertyInfo's as keys in p_validationFields, then you don't need to call GetProperties again.
• You can use var to simplify type declarations: var fieldTypes = new Dictionary<string, FieldType>();. This can make code harder to read when used carelessly, but here the type of fieldTypes is obvious from the right-hand side, and having to spell it out on the left hand is only adding clutter.

• Gae: you are amazing giving me all these advices ! and I really wanna thank you for your extremely and detailed informations , will try to improve my code :) thanks again man
• Gae: Hi ! I've updated the code on 11-10-2017 2100 :)
• Pieter Witvoet: If you want another round of reviews then you'll have to make a new post. You may want to link back to this post to give people some context, and perhaps explain what changes you have made.

Answer 2

Don't take it personal but your approach is not good and will be very difficult to mantain.

Reflection is the way to go but take a look to validation attributes, component model and data annotations.

Here is a link that may point you to a better direction:

http://www.c-sharpcorner.com/UploadFile/ff2f08/creating-custom-validation-attribute-for-data-annotation/

You can create your own validation attributes or use existing and have a Validation class that takes by reflection those attributes and evalute them.

Edit Some reasons why the approach is not good

1) A field can't have multiple validations (mail + required + maxlength,...)

2) Your validation can't compare values with other values (bigger than x, required if,...)

3) Validation class will become soon fat class, as it handles all the validation rules.

4) You will have to duplicate this code every time you want to validate, what could be in multiple places and in multiple applications, if new rules are comming you will have to update all those places.

 y["EMail"] = FieldType.EMail;
 y["DateOfBirth"] = FieldType.DateTime;
 y["Age"] = FieldType.Integer;
 y["PurchasedItems"] = FieldType.Integer;
 y["DateLastPurchase"] = FieldType.DateTime;

5) Validation rules are set somewhere on the fly and it is impossible to figure out what are the validation rules of your data class just by looking at your data class.

• Gae: More clear now. Will try to reply to your points step by step later on, thanks for now :)

Answer 3

I've updated the code. I know, a lot of work still needs to be done, but I've just started changing the code following your tips:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Reflection;
namespace Validation
{
 class Customer
 {
 public object EMail { get; set; }
 public object DateOfBirth { get; set; }
 public object Age { get; set; }
 public object PurchasedItems { get; set; }
 public object DateLastPurchase { get; set; }
 public Customer() {
 this.EMail = "[email protected]";
 //this.DateOfBirth = DateTime.Parse("06/06/1981");
 this.DateOfBirth = "01/12/1981";
 this.Age = "36";
 this.PurchasedItems = "3";
 this.DateLastPurchase= "06/06/2017";
 }
 }
}
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Validation;
using System.Net.Mail;
namespace Validation
{
 class Program
 {
 public static bool CustomValidationLogic(object field)
 {
 return true;
 }
 private delegate bool validationDelegate(object field);
 static void Main(string[] args)
 {
 Customer customer = new Customer();
 Validation validationApp = new Validation(customer);
 Dictionary<string, FieldType> validationFields = validationApp.validationFields;
 validationFields["EMail"] = FieldType.EMail;
 validationFields["DateOfBirth"] = FieldType.DateTime;
 validationFields["Age"] = FieldType.Numeric;
 validationFields["PurchasedItems"] = FieldType.Numeric;
 validationFields["DateLastPurchase"] = FieldType.DateTime;
 validationFields["Age"] = FieldType.Integer;
 validationApp.customValidationHandlers = new Hashtable();
 validationApp.customValidationHandlers.Add(FieldType.EMail, (validationDelegate)CustomValidationLogic);
 object validationResult = validationApp.setValidationFields(validationFields).doValidation();
 }
 }
}
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Net.Mail;
using System.Reflection;
using System.Text.RegularExpressions;
namespace Validation
{
 public enum FieldType
 {
 EMail,
 DateTime,
 Numeric,
 Integer
 }
 public class ValidationResult
 {
 public bool isValid { get; set; }
 public string invalidPropertyName { get; set; }
 public string invalidFieldName { get; set; }
 public ValidationResult()
 {
 this.isValid = false;
 this.invalidPropertyName = string.Empty;
 this.invalidFieldName = string.Empty;
 }
 }
 public class Validation
 {
 private object _toBeValidated { get; set; }
 public Dictionary<string, FieldType> validationFields { get; set; }
 public Hashtable customValidationHandlers { get; set; }
 public Validation(object o)
 {
 this._toBeValidated = o;
 Dictionary<string, FieldType> d = new Dictionary<string, FieldType>();
 this.validationFields = new Dictionary<string, FieldType>();
 }
 public Validation setValidationFields(Dictionary<string, FieldType> f)
 {
 this.validationFields = f;
 return this;
 }
 public ValidationResult doValidation()
 {
 ValidationResult result = new ValidationResult();
 Dictionary<string, FieldType> fields = this.validationFields;
 foreach (PropertyInfo property in this._toBeValidated.GetType().GetProperties())
 {
 FieldType field_type;
 field_type = fields[property.Name];
 if (fields.ContainsKey(property.Name))
 {
 result.isValid = ValidateField(property.GetValue(this._toBeValidated, null), field_type);
 if (!result.isValid)
 {
 result.invalidPropertyName = property.Name;
 result.invalidFieldName = field_type.ToString();
 break;
 }
 }
 }
 return result;
 }
 private Boolean ValidateField(object field, FieldType type)
 {
 Boolean isValid = false;
 switch (type)
 {
 case FieldType.EMail:
 isValid = false;
 // default validation for EMail
 try {
 MailAddress m = new MailAddress((string)field);
 isValid = true;
 }
 catch (FormatException)
 {
 isValid = false;
 }
 // override the default validation for EMail
 if (this.customValidationHandlers != null)
 {
 Delegate handler = (Delegate)this.customValidationHandlers[FieldType.EMail];
 if (handler != null) isValid = (bool)handler.DynamicInvoke(field);
 }
 break;
 case FieldType.DateTime:
 DateTime d;
 isValid = DateTime.TryParse(field.ToString(), out d);
 if (d > DateTime.Now) isValid = false;
 break;
 case FieldType.Numeric:
 isValid = new Regex(@"^[-+]?(\d*\.)?\d+$").IsMatch(field.ToString());
 break;
 case FieldType.Integer:
 int nAsInteger;
 isValid = int.TryParse(field.ToString(), out nAsInteger);
 break;
 }
 return isValid;
 }
 }
}

• Zeta: Did you read the linked article? See point 3: Posting a self-answer: "Describe what you changed, and why.". Furthermore, if you changed much, you probably want your new code reviewed, too. That's also explained in the linked article.
• Gae: I made lot of changes , following the advices coming from the others developers, take me a lot of time giving explanation for the changes. Reading this thread I think is sufficient to understand what has been changed and why
• Igor Soloydenko: @Gae if you don't want to explain the changes, what's the point of posting an answer? Are the readers supposed to make a mental diff exercise? IMO, it's disrespectful to other reviewers to say that it will take you a lot of time to describe the changes. Haven't the reviewers spent a lot of time to help you with your code?
• Gae: OMG. Ok maybe you are right. I just wanted to update the code, believe me, don't wanna be misrespectful with anyone. Just not not having enough time today, but at the same time just wanted to give you the latest version of the code : /
• BenKoshy: @Gae my recommendation is to perhaps read about polymorphism (google it). HTH