Let's assume that our system can perform actions, and that an action requires some parameters to do its work. I have defined the following base class for all actions (simplified for your reading pleasure):
public abstract class BaseBusinessAction<TActionParameters>
: where TActionParameters : IActionParameters
{
protected BaseBusinessAction(TActionParameters actionParameters)
{
if (actionParameters == null)
throw new ArgumentNullException("actionParameters");
this.Parameters = actionParameters;
if (!ParametersAreValid())
throw new ArgumentException("Valid parameters must be supplied", "actionParameters");
}
protected TActionParameters Parameters { get; private set; }
protected abstract bool ParametersAreValid();
public void CommonMethod() { ... }
}
Only a concrete implementation of BaseBusinessAction
knows how to validate that the parameters passed to it are valid, and therefore the
ParametersAreValid
is an abstract function. However, I want the base class constructor to enforce that the parameters passed are always valid, so I've added a
call to ParametersAreValid
to the constructor and I throw an exception when the function returns false
. So far so good, right? Well, no.
Code analysis is telling me to "not call overridable methods in constructors" which actually makes a lot of sense because when the base class's constructor is called
the child class's constructor has not yet been called, and therefore the ParametersAreValid
method may not have access to some critical member variable that the
child class's constructor would set.
So the question is this: How do I improve this design?
Do I add a Func<bool, TActionParameters>
parameter to the base class constructor? If I did:
public class MyAction<MyParameters>
{
public MyAction(MyParameters actionParameters, bool something) : base(actionParameters, ValidateIt)
{
this.something = something;
}
private bool something;
public static bool ValidateIt()
{
return something;
}
}
This would work because ValidateIt
is static, but I don't know... Is there a better way?
Comments are very welcome.
This is a common design challenge in an inheritance hierarchy - how to perform class-dependent behavior in the constructor. The reason code analysis tools flag this as a problem is that the constructor of the derived class has not yet had an opportunity to run at this point, and the call to the virtual method may depend on state that has not been initialized.
So you have a few choices here:
When possible, have each constructor perform the validation. But this isn't always desirable. In that case, I personally, prefer the factory pattern because it keeps the implementation straight forward, and it also provides an interception point where other behavior can be added later (logging, caching, etc). However, sometimes factories don't make sense, and in that case I would seriously consider the fourth option of creating a stand-along validator type.
Here's the code example:
public interface IParamValidator<TParams>
where TParams : IActionParameters
{
bool ValidateParameters( TParams parameters );
}
public abstract class BaseBusinessAction<TActionParameters,TParamValidator>
where TActionParameters : IActionParameters
where TParamValidator : IParamValidator<TActionParameters>, new()
{
protected BaseBusinessAction(TActionParameters actionParameters)
{
if (actionParameters == null)
throw new ArgumentNullException("actionParameters");
// delegate detailed validation to the supplied IParamValidator
var paramValidator = new TParamValidator();
// you may want to implement the throw inside the Validator
// so additional detail can be added...
if( !paramValidator.ValidateParameters( actionParameters ) )
throw new ArgumentException("Valid parameters must be supplied", "actionParameters");
this.Parameters = actionParameters;
}
}
public class MyAction : BaseBusinessAction<MyActionParams,MyActionValidator>
{
// nested validator class
private class MyActionValidator : IParamValidator<MyActionParams>
{
public MyActionValidator() {} // default constructor
// implement appropriate validation logic
public bool ValidateParameters( MyActionParams params ) { return true; /*...*/ }
}
}
If you are deferring to the child class to validate the parameters anyway, why not simply do this in the child class constructor? I understand the principle you are striving for, namely, to enforce that any class that derives from your base class validates its parameters. But even then, users of your base class could simply implement a version of ParametersAreValid()
that simply returns true, in which case, the class has abided by the letter of the contract, but not the spirit.
For me, I usually put this kind of validation at the beginning of whatever method is being called. For example,
public MyAction(MyParameters actionParameters, bool something)
: base(actionParameters)
{
#region Pre-Conditions
if (actionParameters == null) throw new ArgumentNullException();
// Perform additional validation here...
#endregion Pre-Conditions
this.something = something;
}
I hope this helps.
I would recommend applying the Single Responsibility Principle to the problem. It seems that the Action class should be responsible for one thing; executing the action. Given that, the validation should be moved to a separate object which is responsible only for validation. You could possibly use some generic interface such as this to define the validator:
IParameterValidator<TActionParameters>
{
Validate(TActionParameters parameters);
}
You can then add this to your base constructor, and call the validate method there:
protected BaseBusinessAction(IParameterValidator<TActionParameters> validator, TActionParameters actionParameters)
{
if (actionParameters == null)
throw new ArgumentNullException("actionParameters");
this.Parameters = actionParameters;
if (!validator.Validate(actionParameters))
throw new ArgumentException("Valid parameters must be supplied", "actionParameters");
}
There is a nice hidden benefit to this approach, which is it allows you to more easily re-use validation rules that are common across actions. If your using an IoC container, then you can also easily add binding conventions to automatically bind IParameterValidator implementations appropriately based on the type of TActionParameters
I had a very similar issue in the past and I ended up moving the logic to validate parameters to the appropriate ActionParameters
class. This approach would work out of the box if your parameter classes are lined up with BusinessAction classes.
If this is not the case, it gets more painful. You have the following options (I would prefer the first one personally):
IValidatableParameters
. The implementations will be lined up with business actions and will provide validationIf you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With