CODEWITHSUNDEEP
c#
Ɖiamond ǤeezeƦ
Ɖiamond ǤeezeƦ

Reputation: 3331

Suggestions on validating a class and its collection properties

I have a Query class that has a number of properties that are collections of other objects. As part of validating a Query object I want to validate all objects in each of the Query's collections. So far I have come up with three approaches to do this. I would like feedback on which approach people prefer and any other possible approaches.

Approach 1

public static ValidationResult ValidateQuery(Query query)
{
    ValidationResult result;
    result = ValidateColumns(query);
    if (result.Passed)
    {
        result = ValidateFilters(query);
        if (result.Passed)
        {
            result = ValidateSortOrders(query);
        }
    }
    return result;
}

While the number of collection properties is small, the code is readable. But if I had significantly more, I would end up with lot of nested if statements, which in my opinion reduces readability. I try to resolve this in my next two approaches:

Approach 2

public static ValidationResult ValidateQuery(Query query)
{
    ValidationResult[] results = new ValidationResult[4];
    results[0] = ValidateColumns(query);
    results[1] = ValidateFilters(query);
    results[2] = ValidateGroups(query);
    results[3] = ValidateSortOrders(query);  
    return results.FirstOrDefault(item => !item.Passed) ?? results[0];
}

Approach 3

public static ValidationResult ValidateQuery(Query query)
{
    ValidationResult result = null;
    int i = 0;
    bool done = false;
    do
    {
        switch (i)
        {
            case 0: result = ValidateColumns(query); break;
            case 1: result = ValidateGroups(query); break;
            case 2: result = ValidateSortOrders(query); break;
            default: done = true; break;
        }
        ++i;
    } while (result.Passed && !done);
    return result ?? new ValidationResult(true, string.Empty);
}

In case you need it, the definition for the ValidationResult class:

public class ValidationResult
{
    public ValidationResult(bool passed, string message)
    {
        this.Passed = passed;
        this.ErrorMessage = message ?? string.Empty;
    }
    public string ErrorMessage {get; private set; }
    public bool Passed { get; private set; }
}

Upvotes: 3

Views: 250

Answers (5)

Igor Korkhov
Igor Korkhov

Reputation: 8558

You can add a method called And to your ValidationResult class:

public class ValidationResult
{
    public ValidationResult(bool passed, string message)
    {
        this.Passed = passed;
        this.ErrorMessage = message ?? string.Empty;
    }

    public ValidationResult And(ValidationResult other)
    {
        this.Passed &= other.Passed;
        this.Message += "\n" + other.Message;
        return this;
    } 
    public string ErrorMessage {get; private set; }
    public bool Passed { get; private set; }
}

Then you can use it like this:

bool passed = ValidateColumns(query)
                  .And(ValidateGroups(query))
                  .And(ValidateSortOrders(query))
                   //...
                  .And(ValidateSomethingElse(query))
                  .Passed;

Upvotes: 2

Jonathan S.
Jonathan S.

Reputation: 5877

I have done validation similar to Approach 2. But instead of using an array, you could do something like:

    public IEnumerable<ValidationResult> ValidateQuery(Query query)
    {
        if (!ValidateColumns(query)) yield return new ValidationResult("Bad columns");
        if (!ValidateFilters(query)) yield return new ValidationResult("Bad filters");
        if (!ValidateGroups(query)) yield return new ValidationResult("Bad groups");
        if (!ValidateSortOrders(query)) yield return new ValidationResult("Bad sort order");
    }

The benefit of this approach is that you don't have to hardcode the array's size and you can return multiple failed validations. You could then set your "Passed" flag by checking if any results are present using results.Any()

Upvotes: 2

Erik Dietrich
Erik Dietrich

Reputation: 6090

If the order of precedence and number of validations might change, you could use a chain of responsibility pattern:

http://www.dofactory.com/Patterns/PatternChain.aspx

Instead of a static method that invokes static, named sub-methods, you have a base validator and each kind of validation is an inheritor that knows about the next validator in the chain. So, ColumnValidator looks at the query and, if good, passes it along to GroupsValidator, etc. If it fails at any point, the validator decorates the error message and returns, since further validation is unnecessary.

And, as an added bonus, if you do it this way your validation is not done with a static method so you can unit test consumers of the validation code.

Upvotes: 1

Matthew
Matthew

Reputation: 25763

What about yield return?

IEnumerable<string> ValidateAll(Query query)
{
    if( !ValidateSomething() ) {
        yield return "Validate Something Failed...";
    }

    if( !ValidateSomethingelse() ) {
        yield return "Something else failed...";
    }
}

Of course, the enumerable type does not have to be a string.

Upvotes: 2

Matti Virkkunen
Matti Virkkunen

Reputation: 65126

What's wrong with

result = ValidateSomething();
if (!result.Passed)
    return result;

result = ValidateSomethingElse();
if (!result.Passed)
    return result;

That is, if you really want to only return one error. If this is for user input, it results in a rather annoying interface. When the user has made multiple errors, this thing can only report one of them at a time, and the user will have to keep making corrections one by one until they're gone.

Have you considered returning a collection of validation results? You could pass the collection in to each validation method and have them add their possible errors to the collection.

And avoid the while-switch.

Upvotes: 3

Related Questions