3
\$\begingroup\$

I am picking up a code smell from this code and I am not sure of the best way to rid myself of it.

I have a validation class that has a function that returns a list of IValidationRule<T> implementations (RuleOne, RuleTwo, RuleThree).

public interface IValidationRule<T>
{
    ValidationRuleResult Validate(T instance);
}

public override ValidationRules<Order> AllRules() 
=> new ValidationRules<Order>
{
    Rules =
    {
        RuleOne(),
        RuleTwo(),
        new StopProcessingIfInvalidCheckpoint<Order>(),
        RuleThree()
    }
};

As the rules are iterated through, the IValidationRule<T>.Validate method is called on them. They will return their results which are added to a collection.

I implemented a class that inherits from IValidationRule<T> called StopProcessingIfInvalidCheckpoint which will cease the processing of any rules following it if any of the rules before it fail.
I felt this was a clean and visual way to kick out of the validation routine early.

What I don't like is that the StopProcessingIfInvalidCheckpoint class has to be passed the same generic as all the other rules around it. The Validate method on this class is never even called and thus, it does not matter that it knows it's generic is an Order.

Below is the extension method I use to validate a list of rules and you can see where I kick out if I come across the StopProcessingIfInvalidCheckpoint class:

public static ValidationRuleResult Validate<T>(
    this IEnumerable<IValidationRule<T>> validationRules, 
    T instance
)
{
    var validationResults = new List<ValidationResult>();

    foreach (var vr in validationRules)
    {
        if (vr is StopProcessingIfInvalidCheckpoint<T> && validationResults.Any()) { break; }
        var result = vr.Validate(instance);
        if (result.IsValid) { continue; }
        validationResults.AddRange(result.ValidationResults);
    }

    var res = new ValidationRuleResult
    {
        ValidationResults = validationResults,
        IsValid = !validationResults.Any()
    };
    return res;
}

How can I improve this code?

\$\endgroup\$
  • \$\begingroup\$ Do not change the code in the question after receiving an answer. Incorporating advice from an answer into the question violates the question-and-answer nature of this site. \$\endgroup\$ – t3chb0t Sep 1 '16 at 20:18
  • \$\begingroup\$ What should I do when someone answers my question? \$\endgroup\$ – t3chb0t Sep 1 '16 at 20:19
1
\$\begingroup\$

You shouldn't bind the implementation to any particular type in this case the StopProcessingIfInvalidCheckpoint.

I think it would be better to drop this class and instead add a new property IsFinal to the interface. You could check if the validation fails and IsFinal is set to true then you stop processing other rules.

If you want to get rid of the generic argument but at same time you want to keep it for other implementations then add another abstraction layer. Make the interface non-generic and add an abstract class that is derived from the non-generic interface:

interface IValidationRule
{
    ValidationRuleResult Validate(object instance);
    bool IsFinal { get; }
}

abstract class ValidationRule<T> : IValidationRule
{
    public abstract ValidationRuleResult Validate(object instance);
    public bool IsFinal { get; set; }
}

class FooValidationRule : ValidationRule<int>
{
    public override ValidationRuleResult Validate(object instance)
    {
        ... do something with (T)instance;
    }
}

and

foreach (var vr in validationRules)
{
    var result = vr.Validate(instance);
    if (!result.IsValid && vr.IsFinal)
    {
        break;
    }
    validationResults.AddRange(result.ValidationResults);
}

so

Rules =
{
    RuleOne(),
    RuleTwo() { IsFinal = true },        
    RuleThree()
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for your input! I actually have that setting, but did not include it in the code (minimize noise). I use it as "if this rule fails, go no further". I added the specific checkpoint "rule" since I also wanted the flexibility to say, if any rules have failed to this point go no further. \$\endgroup\$ – Airn5475 Sep 1 '16 at 18:57
  • 2
    \$\begingroup\$ @Airn5475 well, you see the result of omitting crucial parts of the code now. you should always post everything you have. I don't think that a few more lines would hurt. and I still don't agree with your implementation because it's tightly coupled to one particular class which is a bad design. \$\endgroup\$ – t3chb0t Sep 1 '16 at 19:02
  • \$\begingroup\$ I updated my example and added the logic that will stop processing further rules if the current rule fails. Again, this logic is only good for stopping the validation of all following rules IF the CURRENT rule fails. (hence the reason I didn't include it) I want to stop processing if any rules up to a certain point have failed. \$\endgroup\$ – Airn5475 Sep 1 '16 at 20:21
  • \$\begingroup\$ @Airn5475 you may not change the code in your question anymore. Please read the rules. It would invalidate the reviews. \$\endgroup\$ – t3chb0t Sep 1 '16 at 20:23
  • \$\begingroup\$ @Airn5475 it's better to ask new question with the complete code. \$\endgroup\$ – t3chb0t Sep 1 '16 at 20:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.