4
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ what does snip mean? And also, is this a MVC WebApi project? \$\endgroup\$ – dreza Jun 29 '14 at 1:53
3
\$\begingroup\$

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
         }
      }
    
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Caster Troy Jun 29 '14 at 16:47
  • \$\begingroup\$ @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() \$\endgroup\$ – dreza Jun 29 '14 at 20:15

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.