Inheritance is Evil: The Epic Fail of the DataAnnotationsModelBinder

The DataAnnotationsModelBinder is a pretty neat little class that enforces validation attributes on models like this one:

public class Product {
    public int Id { get; set; }

    [Required]
    public string Description { get; set; }

    [Required]
    [RegularExpression(@"^\$?\d+(\.(\d{2}))?$")]
    public decimal UnitPrice { get; set; }
}

Just for the record, this is a cool bit of code that was released to the community for backseat drivers like me to use and complain about. I’m really only bitter because I can’t use it. Let’s take a peek at the source code to see why:

/// <summary>
/// An implementation of IModelBinder which is designed to be a replacement
/// default model binder, adding the functionality of validation via the validation
/// attributes in System.ComponentModel.DataAnnotations from .NET 4.0.
/// </summary>
public class DataAnnotationsModelBinder : DefaultModelBinder {
    ...
}

The problem with this class is that it’s not just about validation. It’s more about binding with validation sprinkled in. The author even states this as a design goal with the comment “designed to be a replacement default model binder.” This is a violation of the Single Responsibility Principle and it’s the reason I can’t use it. The DefaultModelBinder does some basic reflection mapping of form elements to model properties. That’s cool, but what about a controller action that takes json, xml, or anything slightly more complicated than what the default binder can handle?

I’ll tell you what happens. You write a custom model binder which is pretty easy and happens to be a pretty rad extension point in the MVC framework. Consider this model binder that deserializes json:

public class JsonModelBinder<T> : IModelBinder {
    private string key;

    public JsonModelBinder(string requestKey) {
        this.key = requestKey;
    }

    public object BindModel(ControllerContext controllerContext, ...) {
        var json = controllerContext.HttpContext.Request[key];
        return new JsonSerializer().Deserialize<T>(json);
    }
}

That’s pretty cool, but now I’ve lost my validation! This my friend is the evil of inheritance and the epic fail of the DataAnnotationsModelBinder. So what’s wrong with inheritance, isn’t that what OOP is all about? The short answer is no (the long answer is also no.)

peter-venkman-ghostbusters

Human sacrifice, dogs and cats living together… mass hysteria! — Dr. Peter Venkman

Inheritance is pretty enticing especially coming from procedural-land and it often looks deceptively elegant. I mean all I need to do is add this one bit of functionality to another other class, right? Well, one of the problems is that inheritance is probably the worst form of coupling you can have. Your base class breaks encapsulation by exposing implementation details to subclasses in the form of protected members. This makes your system rigid and fragile.

The more tragic flaw however is the new subclass brings with it all the baggage and opinion of the inheritance chain. Why does a model that enforces validation attributes care how the model is constructed? The answer is it shouldn’t, and to add insult to injury, the better design is actually easier to implement. Listen closely and you may hear the wise whispers of our forefathers…

Ward Cunningham Martin Fowler Kent Beck Robert Martin

Use the decorator pattern.

Is this really going to solve all my problems? The short answer is yes. Let’s take a look:

public class BetterDataAnnotationsModelBinder : IModelBinder {
    private IModelBinder binder;

    public BetterDataAnnotationsModelBinder(IModelBinder binder) {
        this.binder = binder;
    }

    public object BindModel(ControllerContext controllerContext, ...) {
        var model = binder.BindModel(controllerContext, bindingContext);
        AddValidationErrorsToModelState(model, controllerContext);
        return model;
    }

    ...
}

Now all this class does is enforce validation and we can use whatever model binder we want to construct our object. Let’s wire this up in the Global.asax real quick:

var productModelBinder = new JsonModelBinder<Product>("product");

ModelBinders.Binders.Add(
    typeof(Product),
    new BetterDataAnnotationsModelBinder(productModelBinder));

This is cool for the following reasons:

  1. We are not coupled to the DefaultModelBinder. This means we don’t know or care about the structure and implementation details of it so it’s free to change in the next version of ASP.NET MVC.
  2. The code is simpler because it is only about validation.
  3. The behavior is composable. This means we can add validation to any model binder (like our json model binder) and bolt on more responsibilities with a chain of decorators. How about converting DateTime fields to UTC or striping formatting characters from phone numbers?
  4. It’s easy to test.

You can substitute a decorator for a subclass in most cases and in most cases it’s the right choice. You’re better off making the decorator your default choice and only settle for inheritance when you really need it.

kip

…don’t be jealous that I’ve been chatting online with babes all day. Besides, we both know that I’m training to be a cage fighter. — Kip

8 Comments  »

  1. Asaph says:

    You say:

    “Your base class breaks encapsulation by exposing implementation details to subclasses in the form of protected members”

    Would the situation be any better if we used private members instead of protected?

  2. Michael Valenty says:

    It would be less bad in that your subclass wouldn’t rely on details of the base class, but the more important issue is that you lose out on composition.

  3. tmont says:

    I think the point is that mutable members shouldn’t be exposed to derived class. As in, getFoo() should be public/protected and setFoo() should be private (except in certain cases like a DTO or something).

    But of course, the whole point of inheritance is to reuse code, so a lot of the time you want encapsulation around the object rather than the class. If that’s not what you’re after, than you’re better off with something like a decorator or a composite.

    Inheritance is only evil if used incorrectly, as Mike so eloquently illustrated in the case of the DataAnnotationsModelBinder.

  4. cDima says:

    Awesome post. But how would this call tie into the MVC DataAnnotation model?

    AddValidationErrorsToModelState(model, controllerContext);

  5. Michael Valenty says:

    The function AddValidationErrorsToModelState could for example, loop over the properties of the model and validate the values against the validation attributes. This class shouldn’t also do the model binding, it should stick to the single responsibility of validation. They are unrelated concerns and there clearly are times when I would like one behavior without the other.

    The author went to great lengths to build out an extensible model binder. The problem is that it’s a difficult task to guess all the ways that something might be used. It’s safer to stick to one thing and do it well, that way when new requirements creep up, they can be composed rather than trying to force it into an extensibility model that didn’t consider what you are trying to do.

  6. Scuba. There are dozens of people who own the uninteresting colourless handbags out there but you don鈥檛 have to be one of them. Details: Oversized neoprene tote with

  7. Hello, your articles here Inheritance is Evil: The Epic Fail of the DataAnnotationsModelBinder | Agile at Work to write well, thanks for sharing!

  8. Estaba rebuscando informacion relacionada con blink banamex y halle esta pagina.
    Esta excelente.

Trackbacks/Pingbacks

    1. Refactoring C# Style | Agile at Work
    2. Configuring Decorators with Google Guice | Agile at Work

    RSS feed for comments on this post, TrackBack URI

    Leave a Comment