Additional model validation

Question

public HttpResponseMessage Post(LongUrlModel model)
{
 if (ModelState.IsValid)
 {
 var baseAddress = Request.RequestUri.ToString().TrimEnd('/');
 if (model.LongUrl.StartsWith(baseAddress))
 {
 ModelState.AddModelError("", "You cant shorten a URL that belongs to this domain");
 return Request.CreateErrorResponse(HttpStatusCode.BadRequest, ModelState);
 }
 *snip*
 }
 return Request.CreateErrorResponse(HttpStatusCode.BadRequest, ModelState);
}

I am not at all fond of the conditional logic here, it can be quite hard to follow. How can I refactor the additional model validation logic so that it will co-exist with the standard model validation nicely?

I feel as though there could be a way to refactor this code so that Request.CreateErrorResponse only occurs in the code once, but I cannot see how at the moment.

• dreza: what does snip mean? And also, is this a MVC WebApi project?

Answer 1

In regards to your question:

I feel as though there could be a way to refactor this code so that Request.CreateErrorResponse only occurs in the code once, but I cannot see how at the moment.

I would consider reducing the nesting (Arrow code) of the If statements and perform follow the return early mantra.

public HttpResponseMessage Post(LongUrlModel model)
{
 if (!ModelState.IsValid)
 {
 return BadRequest();
 }
 var baseAddress = Request.RequestUri.ToString().TrimEnd('/');
 if (model.LongUrl.StartsWith(baseAddress))
 {
 ModelState.AddModelError("", "You cant shorten a URL that belongs to this domain");
 return BadRequest();
 }
 // Perform the valid code logic here
}
private HttpResponseMessage BadRequest()
{
 return Request.CreateErrorResponse(HttpStatusCode.BadRequest, ModelState);
}

In regards to your other question

I am not at all fond of the conditional logic here

There's a few things I can think of you could try.

1. Create a custom validation attribute and decorate the LongUrl property.

    public class UrlLengthAttribute: ValidationAttribute
    {
    public UrlLengthAttribute()
    {
    }
    protected override ValidationResult IsValid(object value, ValidationContext validationContext)
    {
    var httpContext = new HttpContextWrapper(HttpContext.Current);
    var url = value as string;
    if(string.isNullOrWhiteSpace())
    {
    return base.IsValid(value, validationContext);
    }
    // I'm not 100% on this code but you get the general idea
    var baseAddress = httpContext.RequestUri.ToString().TrimEnd('/');
    if(url.StartsWith(baseAddress))
    {
    return new ValidationResult(this.FormatErrorMessage(validationContext.DisplayName));
    }
    return null;
    }
    }
   

You can then use this to decorate your model:

 public class LongUrlModel
 {
 [UrlLength(ErrorMessage = "You cant shorten a URL that belongs to this domain")] 
 public string LongUrl { get; set; }
 }

1. You could the extract the model validation out into a separate Validator class and use that in the controller instead. At the very least it might be moving towards the lines of creating a unit testable module.

    public class LongUrlModelValidator
    {
    public LongUrlModelValidator(HttpContextBase httpContext)
    {
    // store and use
    }
    public bool Validate(LongUrlModel model)
    {
    // perform validation logic here
    }
    }
   

• Caster Troy: Thanks bro. You couldn't have known this, but I am using Web API 2 so I used the standard BadRequest method. I also reduced the nesting like you suggested. A custom validation attribute is a nice idea, as is a validator class, but as the validation is only required in one place (the aforementioned action) I do not feel as though the ROI makes it worthwhile.
• dreza: @CasterTroy ah ok, fair enough. The only other suggestion then is perhaps moving that validation to a private method within the controller i.e. if(IsShortUrlRequest()) return BadRequest()