views:

312

answers:

2

Hi all,

I have a question regarding the method i am using for doing Business Rule Validations in asp.net mvc.

Currently i have an exception class that looks something like this

public class ValidationException : Exception 
{
    private ModelStateDictionary State { get; set; }
    public ValidationException(ModelStateDictionary state)
    {
        State = state;
    }
    public void MergeModelStates(ModelStateDictionary state)
    {
        state.Merge(this.State);
    }
}

and a validator that looks like this

public void Validate(IEntity entity)
{
    ModelStateDictionary state = new ModelStateDictionary();
    if (entity.Contact != null && _service.GetBy(entity.Contact.Id) == null)
        state.AddModelError("Contact", "Invalid Contact.");
    if (entity.Title.Length > 8)
        state.AddModelError("title", "Title is too long...");
    ... etc
    if (!state.IsValid)
        throw new ValidationException(state);
}

and a Controller That does something like this

public ActionResult Add()
{
    var entity = new InputModel;
    try
    {
        TryUpdateMode(inputModel);
        ..... Send input to a Repository (Repository calls Validate(entity);
    }
    catch (ValidationException validationException)
    {
       validationException.MergeModelStates(this.ModelState);
       TryUpdateModel(inputModel);
       return View("Add",inputModel);
    }
    return View("List");
}

Is it wrong to use an exception to do something like this? Are there examples of a better method? I don't really want to add the validation to the Model Entity itself. The only other way i've seen it done is to inject the Controllers ModelState to the Repository layer, but that seemed sloppy to me.

Thanks for any help

+7  A: 

Exceptions should generally be for exceptional cases, and not to handle something that could routinely happen during normal execution of your program. There's a lot of good reasons for this - here's some I've run into fairly regularly:

  1. Performance issues - Exceptions are generally fairly expensive operations - if they are thrown regularly your performance may suffer.
  2. Dealing with uncaught validation exceptions - In the event that you happen to use your code without dealing with the exception, you'll surface your validation error up as far as a "yellow screen" or a crash handler - probably not the best user experience.
  3. Exceptions aren't structured in a way that allows for good user-facing information. Take a look at the exception class - not much there is set up in a way that makes for good user-facing information, something you'll need for relaying the information back to the user. Any time I've tried to use exceptions in this manner I've ended up with a whole host of subclasses with properties tacked on that don't really make a lot of sense belonging to an exception.

One approach I usually like to do is to provide a public Validate method that returns a list of errors (but never throws an exception itself), and then a Save method which calls Validate() and throws an exception if there are any errors. You switch the behavior from "throw if the model isn't valid" to "throw if the code tries to save while the model is in an invalid state".

To address the comment below regarding performance of throws in Validate vs. Save - throwing in Save() will have the exact same performance penalty as throwing in Validate(). The key difference however is that this should never happen - you're guarding against a developer using your classes improperly rather than using exceptions as a validation method. Properly written, code that calls the save method should look something like:

ValidationResult result = obj.Validate();
if (result.IsValid) {
   obj.Save();
} else {
   // display errors to the user
}

The exception would only be thrown if the developer forgot to check the validation status before saving. This has the benefit of both allowing validation to happen without using exceptions, as well as protecting your database by never allowing invalid entities to be saved. Ideally you wouldn't catch the exception in the controller at all and let general error handling routines deal with it, since the problem is no longer with user input but instead the fault of the developer.

Ryan Brunner
Thanks! This does seem cleaner to me since the ModelStateDictionary isn't used in any other class except the controller.Is there a performance difference between throwing from the Save method rather than from the validate method? Oh and once the exception is thrown and caught i would need to Call Validate and get all the errors and add them to the model state right?
AlteredConcept
Alright so i'll have to validate twice once in the controller and once again in the save method. Crap i wanted to do all my validation in the repository layer but i guess i have to do it in the Controller as well. oh well.. Thanks!
AlteredConcept
+2  A: 

Ideally you'd use the data annotation feature in ASP.NET MVC 2:-

http://stephenwalther.com/blog/archive/2008/09/10/asp-net-mvc-tip-43-use-data-annotation-validators.aspx

http://weblogs.asp.net/scottgu/archive/2009/07/31/asp-net-mvc-v2-preview-1-released.aspx

For example:-

public class Person
{
  [Required(ErrorMessage="Please enter a name.")]
  public String Name { get; set; }
}

If you're not up for upgrading, there's still a solution:

If you pass a reference to the Controller's ModelState dictionary (wrap it up inside an interface if you're worried about separation of concerns) to your Validator, call AddModelError if you find errors, then in your Controller you can call if (ModelState.IsValid) and take appropriate action:-

var entity = new InputModel();

TryUpdateModel(entity);
MyValidator.Validate(entity, ModelState);

if(ModelState.IsValid) { ...

and your Validator looks like:-

public void Validate(IEntity entity, ModelStateDictionary state)
{
  if (entity.Contact != null && _service.ValidId(entity.Contact.Id) == null)
    state.AddModelError("Contact", "Invalid Contact.");
  // etc
}
Iain Galloway