Object Validation

[Three5Eight]

Cool, that is similar to what I was thinking about doing. I wasn't sure if it was a good idea or not, but now I know its used by other people.

Was the last code example I posted ok? Is it normal to leave a function with no implementation like I have done?

Also, like CPUWizard said I want to keep my code from duplicating as much as possible. On a type like boolean, which doesn't need to be validated because its always either true or false guranteed. Should I be putting my get; set; accessors in the class itself, or should I be making another class similar to ValidatedString that will take care of the accessors? That would reduce duplication.

[Three5Eight]

OK! Finally!

I got it done. I added some extra classes for data types that I will be using. Maybe I don't need to do this but it makes my code so much easier to read because its not loaded with get; set; anymore.

Here's what I did:

   public class BooleanField
    {
        private bool m_value;

        public bool Value
        {
            get { return m_value; }
            set { m_value = value; }
        }
    }

    public class GuidField
    {
        private Guid m_value;

        public Guid Value
        {
            get { return m_value; }
            set { m_value = value; }
        }
    }

        public readonly BooleanField Deleted;
        public readonly BooleanField Taxable;  
        public readonly GuidField InternalID;

// constructor
            Deleted = new BooleanField();
            Taxable = new BooleanField();
            InternalID = new GuidField();

I put these extra classes in a namespace called "Helpers"/

Now I can access everything with Value. I have 15 booleans in one class for various settings and I don't have to type the accessors over 15 times, omg that saves me so much time!

In the future if I need to add validation to the Guid field I can simply modify the GuildField class. The entire program will update automatically.

[TheCPUWizard]

[QUOTE=Three5Eight;1790619]

EVERY validator should have one method. Validate(...) DO NOT change the method name, just change the class name.

I should NOT have declared the base class abstract, it should in fact be a do nothing validator. This will actually result in faster code than having to check if a validator is installed.

[Three5Eight]

But now I'm confused because the Validator base class can't have the same methods?

    public abstract class Validator
    {
        public virtual void ValidateString(string m_string) { }
        public virtual void ValidateInt(int m_int) { }
    }

Are you saying the Validator class should just be an empty class?


Edit:

If this is correct then I need to modify the base-class:

    public class Validator
    {
        public virtual void Validate(object o) { }
    }

[BigEd781]

You can override the Validate() method in your inherited classes so that it does the right thing for that class..

[TheCPUWizard]

But now I'm confused because the Validator base class can't have the same methods?

Einstein once said, keep things as simply as possible, but no simpler...

In this case, my keeping generics out of the picture to keep things simple has actually caused the confusion.


If you are rady, try:

class Field<T>
{
  public  T Value
        {
            get { return this.m_Value; }
            set
            {
                if (!m_Validator.Validate(value(value))
                {
                    throw new ArgumentException();
                }
                m_Value= value;
            }
        }
     public Field(Validator<T> validator)
     {
          m_Validator = validator;
     }
    private T m_Value;
    private Validator<T> m_Validator;
}


class Validator<T> 
{
     public virtual void Validate(T data) {} // do nothing base...
}

class ValidatorNumRange : Validator<Int>
{
   public virtual void Validate(Int data) 
   {
   // do work here
   } 
}

Does that make sense???

[Three5Eight]

Does that make sense???

Yes, however one part doesn't make sense to me.

 public  T Value
        {
            get { return this.m_Value; }
            set
            {
                if (!m_Validator.Validate(value(value))
                {
                    throw new ArgumentException();
                }
                m_Value= value;
            }
        }

I'm not sure what it means (value(value)). "value" is a keyword so I don't know if its getting the value or value .. doesn't make sense to me.

Also, that statement will never work because Validate does not return a value so ! is not making any sense to me.

[Three5Eight]

If I modify Validate to return a bool so that statement will compile, then the class is incorrect because my override would also have to return a bool and it would not return anything because once that exception is thrown the function is done.

        public override void Validate(int p_int)
        {
            if ((p_int < m_LBound) || (p_int > m_UBound))
            {
                throw new ArgumentOutOfRangeException();
                return false; // never happen
            }
        }

I'll keep working on it.

New Version: But it looks wierd.

    public class Validator<T>
    {
        public virtual bool Validate(T data) { return false; }
    }

        public override bool Validate(string p_string)
        {
            if (string.IsNullOrEmpty(p_string))
            {
                throw new ArgumentNullException(); // RETURN NOTHING
            }

            if ((p_string.Length < m_minLength) || (p_string.Length > m_maxLength))
            {
                throw new ArgumentOutOfRangeException(); // RETURN NOTHING
            }
            return true;
        }

Ok, I believe I got it. Here is the final solution attached for people to use as long as CPUWizard thinks its correct this time.

The only thing I have left to do is figure out how to add an IsValid property to the Person class, that wont require me to write all the data validation functions over again. I'll work on that tomorrow.

[TheCPUWizard]

The return value is in case you eventually need a condition where you will suppress the assignment silently. Until this need arises (which will come with some advanced validators!), you should just implement so that it either throws, or returns true.

[Three5Eight]

OK,

I added one function to the Field<> class:

        public bool IsValid()
        {
            if (m_validator != null)
            {
                if (!m_validator.Validate(m_value))
                {
                    return false;
                }
            }
            return true;
        }

I also use that function with the IsValid() function of the Person class (in this case Department class):

        public bool IsValid()
        {
            bool validationError = false;

            if (!Description.IsValid()) { validationError = true; }
            if (!ReportName.IsValid()) { validationError = true; }
            if (!DisplayName.IsValid()) { validationError = true; }         
            if (!DepartmentID.IsValid()) { validationError = true; }            

            return !validationError;
        }

I believe that was the final touch, everything should be working now.

Thanks for all the help on this matter!

[TheCPUWizard]

Glad you have a workable solution.

I see you added a check to allow a field to NOT have a validator (the check for null). I believe this to be a bad decision.

1) It makes it easier to forget to install a valiadtor (remember to strive to make code HARD to use incorrectly). It is better that the coder must explicitly create the validator (which is why the base classes I gave were NOT abstract, so they could be used as explicit "do nothing".

2) It can reduce performance. As this pattern becomes more and more common in your designeds, you may find the "Field" code to be executing millions of times per seconds. Optimal performance is achieved by ELIMINATING branches which flush time compilers pipelines. It will actually be faster to call a "do nothing" than to check for null in many circumstances.

[Three5Eight]

Well the reason I decided not to have a validator is because some fields cannot be validated. For example:

bool Taxable;

In my Department class to tell if the class is taxable or non-taxable. I don't know how to validate that, because its a boolean and it can never be invalid. It can't be assigned to null and it is guranteed to always be true or false. I don't see how that could be invalid.

So if I were to validate that field I would have to check if its either true or false and it will always be one of the two values.

Also, a GUID field I have no way to validate. I just make sure that its not Guid.Empty. But there is no way that a GUID can be invalid either because you would receive a cast exception as soon as you tried to assign it something that is not a real GUID...

So I guess you want me to create a BooleanValidator class and just have that class do nothing??

[TheCPUWizard]

But then you make things "Harder to Use Correctly". Some of your objects "Fields" will have one syntax, others wil have a different one:

class Foo
{
    public Field<Int> Item1;
    public boo Item2;
}

Foo f;
f.Item1.Value = 3;   // need to use value since this is a validated field.
f.Item2 = true;l       // raw field, cant use value.

Also your object level validation will be more difficult (because you can not build up a simple collection of all of the fields and then do a for-each on the IsValid method).

Also if you ever need to change from a raw field to a validated field, then all of your client code will have to change.

Remember the goal (as Scott Meyers put so well about 13 years ago) is to "PRogram in the Future Tense". This means writing your code today, so that it can meet the (currently unknown) challenges of tomorrow.

So far we have simply been talking about some trivial "value" type validations. What happens if next year you need to validate a user (or better a role) for security to assign a valud to a field?

If you write all of your code today to use the new classes, then you can simply enhance your validators and NONE of your application level code will need to change (e.g Only a "Supervisor" can mark a item as non-Taxable in an inventory control system).

[Three5Eight]

Ok, so I think you are saying instead of doing this:

public Field<bool> field1;

I just use the normal way?

public Field<string> description;
public bool taxable;

So when I am using a field that doesn't need to be validated then don't use the Field class?

If that's what you are saying I tried to do that but then I got into code duplication, because if I have 6 boolean fields in my class then I have to type 6 get; set; accessors, one for each field. So the reason I put them into Field and didn't assign a validator was so that I could use the get; set; included in Field, instead of typing them over and over again.

[TheCPUWizard]

Ok, so I think you are saying instead of doing this:

public Field<bool> field1;

I just use the normal way?

public Field<string> description;
public bool taxable;

No...that leads directly to the "Negatives" I pointed out in my previous post.

what I am saying (at least for how I have been using this pattern for the past decade)

1) Alway use "public readonly Field<T> fieldname"

2) ALWAYS have a validator assigned.

3) DO NOT have an "if (validator==null)" in the setter within the field class.