Should validation for different block of codes be combined?

Question

I have a function in my C# class :

class Fun {
    private string waterGun;
    private string jacket;

    void HaveSomeFun(bool summers) {
        waterGun = <Some Value>
        jacket = <Some Other Value>

        validate();
        if(summers) {
            Console.WriteLine("Using {0}", waterGun);
        } else {
            Console.WriteLine("Using {0}", jacket);
        }
    }

    private void validate() {
        ArgumentValidationHelper.ValidateNotNullOrEmpty("WaterGun", this.waterGun);
        ArgumentValidationHelper.ValidateNotNullOrEmpty("Jacket", this.jacket);
    }
}

I am making the variables instance members just for validation. Does it make sense to increase the scope of variables from a method to a class just for validation ?

Is there any other (better) way for achieving this functionality where the validation is centralized and variables are not required to be class members ?

If you are brave enough you can use [System.ComponentModel.DataAnnotations](https://msdn.microsoft.com/en-us/library/system.componentmodel.dataannotations.validator(v=vs.110).aspx). [Here](http://stackoverflow.com/q/2109423/6138713) is an answered question related to data annotations. — jegtugado, Apr 15 '16 at 00:28

smead · Answer 1 · 2016-04-15T00:46:31.820

First, I'm not sure why your member variables are private. Is that a typo? If the user can't change them then you don't need to validate them.

Assuming you actually meant public, you could consider converting the member variables into Properties and doing the validation inside the setter. Then you can either throw an error or set it to some default value. I like this approach better because it ensures that your class is always in a valid state (assuming the calling code handles the errors sensibly...), rather than having to call validate() every time you use it.

If you want to leave them as arguments, you could also make a helper class:

class FunArguments {
    private string waterGun;
    public string WaterGun { 
       get { return waterGun; }
       set { 
            //do some validation check here
            waterGun = value;
       }
    }
    // etc..
}

which you can then pass to Fun in some way.

My question is : Does it make sense to increase the scope of variables from a method to a class just for validation ? You are suggesting to make them instance members. Is there a reason for that ? — Amber, Apr 15 '16 at 02:05
Well it depends where your variables come from, which I can't tell from your code. Just writing `` is not very clear. — smead, Apr 15 '16 at 05:12

score 0 · Accepted Answer · answered Apr 15 '16 at 16:16

According to your code validation logic is already centralized (I hope that was the whole intention when you/someone created ArgumentValidationHelper class. Moreover adding a Validate method just to wrap these 2 validations, I feel, is unnecessary unless you need to have additional stuff inside it.

I feel, you know that those class variable are not required, and yes they do not make any sense because they are (A) private (B) not going to be set by calling code.

So my version of your code is:

class Fun {
    void HaveSomeFun(bool summers) {
        string waterGun = <Some Value>
        string jacket = <Some Other Value>

        ArgumentValidationHelper.ValidateNotNullOrEmpty("WaterGun", waterGun);
        ArgumentValidationHelper.ValidateNotNullOrEmpty("Jacket", jacket);

        if(summers) {
            Console.WriteLine("Using {0}", waterGun);
        } 
        else {
            Console.WriteLine("Using {0}", jacket);
        }
    }
}

This sounds reasonable to me. I guess increasing scope of variables from method to a class just for validation is not a good strategy all-together. — Amber, Apr 15 '16 at 18:07

Should validation for different block of codes be combined?

2 Answers2