Warning: Using the IDataErrorInfo feature in an Asp.net MVC application should be considered a Worst Practice.

Posted by Eric Hexter on January 31, 2009

I wanted to comment on the use of IDataErrorInfo in the Asp.Net Mvc Release Candidate.   Take a look at a sample that David Hayden put together to demonstrate how to  use this feature.

 

The updateModel function  will throw an InvalidOperationException if a member of the IDataErrorInfo returns a value indicating there was a validation error.  I call this Exception Based Programming and I believe this is a horrible practice.  Exceptions should be for exceptional cases, not a validation error which is a common ocurance in a line of business appliaction.  I would propose that a better way to do this would be as follows:

I think this is a better way to handle this sort of code because it is very obvious about the call to IsValid .  This does not deal with any exception handling non-sense thorugh the introduction of a ValidateModel filter.  The validation step can be run and ModelState is populated before the real business starts.

The ValidateModel filter is part of the Code Camp Server sample application which you can find at www.CodeCampServer.org , it is a open source sample application build on top of the Asp.Net MVC framework.

Closing Words:

Do not write code that explicitly throws exceptions as part of the normal opperations of an application.  This is bad for so many reasons but the worst reason is that the code is not obvious in what and why it is acting that way.

Comments ?

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Eric Hexter

I am an Architect for Dell.com on the Engineering Excellence team. I (co)Founded MvcContrib, Should, Solution Factory, and Pstrami open source projects. I have co-authored MVC 2 in Action, MVC3 in Action, and MVC 4 in Action. I co-founded online events like mvcConf, aspConf, and Community for MVC. I am also a Microsoft MVP in ASP.Net.
This entry was posted in .Net, Asp.Net, Asp.Net MVC, mvc. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://www.georan.de/georg Georg Wächter

    I thought exactly the same when watching similar code fragments. Very good blog post! This technique is not only better but also faster in terms of performance.

  • Andy Hitchman

    I’ve talked about business rule validation with exceptions before on the Alt.Net forum, For me, the try catch example has much clearer intent. The exception should be a class designed for talking about validation errors rather than a BCL exception.

    I’m not so keen on attribute based validation, as it does tend to encourage CRUD behaviour, however the xVal framework is making me consider at this prejudice again.

  • Colin

    Performance in this context is pretty irrelevant. I think this discussion really depends on whether you think an exception is for genuinely exceptional circumstances or just for errors. That discussion is dependant on both semantics and context. In either case, describing it as a worst practise is a bit dramatic.

  • http://www.lostechies.com/members/erichexter/default.aspx erichexter

    @Andy, I agree that the attribute based validation is a bit questionable, but that could be replaced with a call to a IValidator.Validate(form), either way I see two levels of validation the first which is dealing with datatypes being correct when comming over the http protocol. The second level of validation should be in your domain, say making sure a username does not already exist. This is something which is a business rule and must be validated but it feels dirty if this is not made into a domain level concept.

  • http://www.lostechies.com/members/erichexter/default.aspx erichexter

    @Colin, dramatic… maybe. I suffered through using Commerce Server on .Net 1.1. I know what Microsoft is capable of delivering and it can be horrible. I believe that the use of exceptions for something that should be handled with a call to a method and a clear branch of logic to handle the appropriate counter action is best.

    I can understand that you may feel this practice is ok.
    Unless someone can convince me otherwise, if I do not recomend this method of validation than what should I call it ?… a Bad practice instead of a Worst practice ?

    I figure if I do not make my point clear than as developers who are new to mvc search for validation patterns, they are only going to see these horrible examples of using IDataErrorInfo. I wanted to present a clear and concise argument of why not to use this pattern.

  • http://www.lostechies.com/members/schambers/default.aspx schambers

    I dislike exception based programming as well. Exceptions IMO are used for EXCEPTIONAL circumstances. If data is missing then this is a basic domain validation, not an invalidoperationexception.

    I’ve maintained large amounts of exception based programming code and it’s not pretty when you’re throwing exceptions all over the place to control flow.

  • Mark

    @Colin:

    So if you have 100,000 users all submitting invalid forms and this performance would be an issue ??

  • http://LeeCampbell.blogspot.com Lee Campbell

    I agree that using exceptions to drive the flow of your program is not good practice. However saying that exceptions are for exceptional circumstances is …a bit weak. An exception should be thrown if a violation of a contract occurs. In these examples that would be if the Save method was called on a model that was invalid. However you avoid the exception-based-flow by having an IsValid property/method.
    A tiny nit-pick I still have with all this MVC stuff I have seen is the use of string literals everywhere. Can “edit” be replaced with a const/enum/static-readonly for better discoverablility, ease of refactoring and compile time warning.

  • http://webgambit.com Karthik Hariharan

    Exceptions are pretty terrible for performance. If you do a simple test with Timers to see how costly they are, you will quickly move away from putting them in commonly executed code. If you throw a lot of exceptions based on normal user input, you will be killing the performance of your site unnecessarily.

  • http://nathan.whiteboard-it.com Nathan Stott

    If you actually run a profile, exceptions may eat a lot of resources in debug mode, but in release mode they are insignificant.

    Also, it is possible to use the IDataErrorInfo without using UpdateModel and having it throw an exception. I am using parameter bound models as in your second example with IDataErrorInfo and it doesn’t throw an exception on me if there are errors. It simply sets ModelState.IsValid to false.

  • http://blog.codeville.net Steve Sanderson

    @Karthik
    > Exceptions are pretty terrible for performance. If you do a simple test with Timers…

    I have done exactly that and found that performance is absolutely fine. Did you? As long as you don’t have a debugger attached, you can easily accommodate 10,000 – 100,000 exceptions per second, so setting up some logic that is likely to cause at most a few exceptions per second in total can’t be rejected on performance grounds.

    @various commenters
    Who came up with the idea that exceptions should be reserved for “exceptional” circumstances, and what is exceptional anyway? The .NET framework class libraries establish a clear pattern for the semantics of an exception. In .NET, an exception means “this method could not complete the operation that its name describes” and that is why they’re always used to signal an incomplete or failed operation (and why we don’t use Win32-style return codes).

    I’m not saying that IDataErrorInfo is great (I would certainly change it in a couple of ways if I had the chance) but rather trying to understand why people don’t want to use exceptions even for scenarios that match their intended use.

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Steve

    It’s just that exceptions as _control flow_ logic are a very poor choice. It’s like the old On Error Resume Next in VB6. There are easier ways to indicate that an operation failed, and provide better options for consumers of APIs, than throwing an exception.

  • Rob

    @steve

    I think the idea that exceptions should be reserved for “exceptional” circumstances is a vestige of the original guidance for using exceptions in C++ (as is the notion that using exceptions incurs a large performance penalty). I believe that even in the C++ world, that guidance is no longer as absolute as it once was.

    @bogardj

    Actually, using exceptions to separate the concerns of error handling and main logic flow is a very good choice. For a nice exposition on that take a look at Robert Martin’s Clean Code and the section on Error Handling that Michael Feathers wrote.

    Additionally, many would consider the use of an IsValid method on domain objects to be a poor practice. The argument here is that domain objects should not be allowed to have an invalid state (i.e., have their invarients violated) at any point in time.

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Rob

    Pssh!!! What do those guys know.

    I like an “OperationResult” object, with Success and Errors as attributes. It lets me compose operations much better, and I don’t have to resort to an IsValid.

    Exceptions are annoying as control flow logic simply because they are neither obvious nor discoverable as users of a framework or API. If something fails and I need to take action, tell me directly. There is nothing in C# that allows me to know what exceptions “might” get thrown, so I can handle them appropriately.

  • Rob

    @bogardj

    Care to post an example (or a link to one) of how you use your OperationResult and how it allows you to compose operations much better?

    I’m pretty leery of 3rd party frameworks that don’t tell me what exceptions they throw, and if I have to use one that doesn’t divulge that information, I’ll wrap its API to exert some control over what the rest of my application has to deal with.

    If I or my team wrote the framework (which is usually the case for the application domain model), we know what exceptions we’re going to get.