2
\$\begingroup\$

I have a question regarding whether it is viable or deemed correct if a validation object invoked a query. Essentially, I have some code that looks like this:

public interface ICountryCodeValidator
{
    IEnumberable<ValidationError> Validate(string countryCode);
}

public class CountryCodeValidator : ICountryCodeValidator
{
    private readonly IGetCountryCodeQuery getCountryCodeQuery;

    public CountryCodeValidator(IGetCountryCodeQuery getCountryCodeQuery)
    {
        this.getCountryCodeQuery = getCountryCodeQuery;
    }

public IEnumberable<ValidationError> Validate(string countryCode)
{
    var errors = new List<ValidationError>();

    if (string.IsNullOrEmpty(countryCode) || countryCode.Length > 2)
    {
        errors.Add(new ValidationError { Code = "InvalidRequest", Message = "Country code is invalid." });
    }
    else
    {
        if (this.getCountryCodeQuery(countryCode) == null)
        {
            errors.Add(new ValidationError { Code = "NotFound", Message = "CountryCode not found." });
        }       
    }

    return errors;
}
}

Immediately, I am thinking that CountryCodeValidator responsibilities are growing and whether it should be querying the DB is debatable. Is it perfectly OK for an object simply responsible for validation to perform such an operation?

Further elaborating on this, I have unit tests surrounding CountryCodeValidator and assert against the error codes returned ensuring the correct codes are returned depending on the input (which is fine).

The trouble is, within my Web API controller I have to perform checks on the error code being returned back and depending on the error code create the appropriate status code. For example:

public class CountriesContoller : ApiController
{
    private readonly ICountryCodeValidator countryCodeValidator;
    private readonly ICountryCodeRepository countryCodeRepository;

    public CountriesContoller(ICountryCodeValidator CountryCodeValidator, ICountryCodeRepository CountryCodeRepository)
    {
        this.countryCodeValidator = countryCodeValidator;
        this.countryCodeRepository = countryCodeRepository;
    }

    public HttpResponseMessage Get([FromUri]string countryCode)
    {
        var errors = this.countryCodeValidator.Validate(countryCode);

        if (errors.Any())
        {
            //Code smell?
            var countryNotFoundError = errors.FirstOrDefault(x => x.Code == "NotFound");

            if (countryNotFoundError != null)
            {
                return Request.CreateResponse(HttpStatusCode.NotFound, countryNotFoundError);
            }

            return Request.CreateResponse(HttpStatusCode.BadRequest, errors);
        }

        return Request.CreateResponse(HttpStatusCode.OK, this.countryCodeRepository(countryCode));
    }
}

I guess, the bit I am concerned with is the fact I am leaking the error code definitions in each of my domains and tests.

  • Web API layer I am performing a linq query to check if this error has been added
  • Domain layer needs to know about this due to validation
  • Tests are aware as I am testing whether a NotFound is invoked, so when I mock the CountryCodeValidator I am ensuring it returns a collection of errors with a particular error code.

A quick easy thing to do is create constants or an enum so I am not using magic strings but still this doesn't seem right.

I realise I have 2 calls to the database being made:

  • One to verify the call is not null
  • Second to get the country and return it back

This is simply pseudo code.

\$\endgroup\$
  • \$\begingroup\$ You could simply set properties in your validator but I must say I question the use; why not just have those validations inside your methods? It's not like you're saving out on code or complexity. \$\endgroup\$ – Jeroen Vannevel Jul 11 '14 at 11:11
  • 1
    \$\begingroup\$ The validation of a country code is consumed in several places thus extracting it into its own separate piece of logic sounded reasonable to do. I didn't want to resort to null checks in my code, i.e. duplication. \$\endgroup\$ – Dr Schizo Jul 11 '14 at 13:26
  • \$\begingroup\$ I would imagine country codes are all well-defined in advance, so it may be best to retrieve all country codes up-front when constructing a CountryCodeValidator and pass them rather than an object which issues queries. \$\endgroup\$ – Dan Lyons Jul 11 '14 at 18:18
1
\$\begingroup\$

I believe it's ok to use db data inside a validator's body. As long as all you do with database is reading data, and this data is neccesary for validation. But if you're about to change something in database - that's pretty considerable.

Then, if you can do something with this data, like caching it or smth, you may want to consider moving this part of validator's logic into some abstraction, like CountryCodeHandler or something like this.

I would also point out on your working with string values. I don't think it's good to use those "Not found" or "InvalidRequest" strings. If the string is exactly what you need, i'd advise you to use some static class:

public static class MyHttpStatusCode
{
    public const string NotFound = "Not Found";
    public const string InvalidRequest = "Invalid Request";
}

Thus, you'll get rid of duplicates in your code. Or, if those strings aren't that unavoidable, you can just use HttpStatusCode enum from System.Web (as i recall).

Also in modern versions of C# you don't need the "this" keyword to access to fields and properties those names arent match any variable inside the method or constructor. Unless it's your codestyle that declares such thing.

As for your controller part, i'd recommend you to use the Chain of Responsibility pattern. It will allow you to extend validation results and handle them properly.

public interface ICountryCodeValidationResultFilter
{
    ValidationError[] Filter(List<ValidationError> errors, out HttpStatusCode statusCode);
}
public interface IConcreteCodeValidationResultFilter
{
    bool CanFilter(List<ValidationError> errors);
    ValidationError[] Filter(List<ValidationError> errors, out HttpStatusCode statusCode);
}

public class CountryCodeValidationResultFilter : ICountryCodeValidationResultFilter
{
    private readonly IConcreteCodeValidationResultFilter[] filters;

    public CountryCodeValidationResultFilter(IConcreteCodeValidationResultFilter[] filters)
    {
        this.filters = filters;
    }

    public ValidationError[] Filter(List<ValidationError> errors, out HttpStatusCode statusCode) 
    {
        if (errors.Any())
        {
            for (var filter in this.filters) // you may just write "filters" here
            {
                if (filter.CanFilter(errors))
                {
                    return filter.Filter(errors, out statusCode);
                }
            }
        }

        statusCode = HttpStatusCode.OK;
        return new ValidationError[0];
    }
}

public NotFoundCodeValidationResultFilter : IConcreteCodeValidationResultFilter
{
    public bool CanFilter(List<ValidationError> errors)
    {
        return errors.Any(e => e.Code == "Not Found");
    }

    public ValidationError[] Filter(List<ValidationError> errors, out HttpStatusCode statusCode)
    {
        statusCode = HttpStatusCode.NotFound;
        return new[] { errors.First(e => e.Code == "Not Found") };
    }
}

public InvalidRequestCodeValidationResultFilter : IConcreteCodeValidationResultFilter
{
    public bool CanFilter(List<ValidationError> errors)
    {
        return errors.All(e => e.Code != "Not Found");
    }

    public ValidationError[] Filter(List<ValidationError> errors, out HttpStatusCode statusCode)
    {
        statusCode = HttpStatusCode.NotFound;
        return errors.ToArray();
    }
}

Thus, your controller will look like this:

public class CountriesContoller : ApiController
{
    private readonly ICountryCodeValidator countryCodeValidator;
    private readonly ICountryCodeRepository countryCodeRepository;
    private readonly ICountryCodeValidationResultFilter countryCodeValidationResultFilter;

    public CountriesContoller(
        ICountryCodeValidator countryCodeValidator,
        ICountryCodeRepository countryCodeRepository,
        ICountryCodeValidationResultFilter countryCodeValidationResultFilter)
    {
        this.countryCodeValidator = countryCodeValidator;
        this.countryCodeRepository = countryCodeRepository;
        this.countryCodeValidationResultFilter = countryCodeValidationResultFilter;
    }

    public HttpResponseMessage Get([FromUri]string countryCode)
    {
        var errors = countryCodeValidator.Validate(countryCode);
        HttpStatusCode resultStatusCode;
        var filteredErrors = countryCodeValidationResultFilter.Filter(errors, out resultStatusCode);

        return Request.CreateResponse(resultStatusCode, filteredErrors.Any() ? filteredErrors : countryCodeRepository(countryCode));
        // i believe countryCodeRepository is not just Func, huh? perhabs it should be "countryCodeRepository.Get(countryCode)" ?
    }
}
\$\endgroup\$
  • \$\begingroup\$ I like the use of the Chain of Responsibility pattern, didn't think of handling validation in such a way. The use of the magic string I did mention in my post that it was pseudo code and I would never do that :) Thanks for the input I'll refactor and adopt this pattern \$\endgroup\$ – Dr Schizo Jul 16 '14 at 10:25

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.