PersonViewModel inherits from Person - clever or code smell?

Question

In my ASP.NET app, I have a Person and a PersonViewModel.

My Person was generated by LinqToSql. It has the properties that will be persisted in the database.

My PersonViewModel has everything Person has, plus a couple "select lists", which will be used to populate combo boxes, as well as a FormattedPhoneNumber (which is basically a PhoneNumber with dashes and parenthases added).

I originally just made Person a property on my PersonViewModel, but I was thinking that would mean the page would have to "know" whether something was a Person property or PersonViewModel property. For example, for name, the view would request pvm.Person.Name, but for the phone number, the view would request pvm.FormattedPhoneNumber. If I use inheritance, then everything the view needs would always be a direct property of the view model, hence pvm.Name.

This sounds great, but, there is no real "is-a" relationship here (i.e., I don't really think it makes sense to say "a PersonViewModel is a Person), and it seems to fly in the face of "preferring composition over inheritance". Still, I'm having difficulty thinking of a scenario where I'd need the ability to swap out a Person for something else. If I do this, it would no longer be a PersonViewModel.

What say you? Inherit from Person or keep Person as a property (or something else entirely)? Why?

Update

Thanks for all the answers.

It looks like the inheritance idea has been almost universally rejected, and for some sound reasons:

1. Decoupling the classes allows the ViewModel to contain only the properties from the domain model that are needed, plus any additional properties. With inheritance, you naturally expose all public properties from the domain model, which may not be a good idea.

2. The ViewModel doesn't automatically need to change just because the domain model changed.

3. As Jay mentioned, decoupling the ViewModel facilitates view-specific validation.

4. As Kieth mentioned, using a Mapper (such as AutoMapper) can eliminate a lot of the tedious work in mapping common properties between classes.

Answer 1

Deriving from an object implies an "is a" relationship. So in this case you would effectively be saying that a PersonViewModel is a Person. It seems that this is not likely to be the semantics that you really want to convey, so use composition instead of inheritance here.

In reality it probably doesn't really affect the maintainability of the application (other than having a few more dots here and there) but using composition certainly feels cleaner to me. In addition, if it's a PersonViewModel then presumably the view should know that that's the type it is dealing with.

Answer 2

Make Person a private member, and expose the properties that your view requires in the PersonViewModel, even if those properties just pass through the corresponding property of Person.

pvm.Person.Name is the code smell here.

---

Edit:

You're also limiting yourself to client-side validation and/or validation in the domain model, the latter meaning that if you create a second or subsequent ViewModel for Person, you can't change the validation on Person's members. (Well, you could, but this is a violation of the open-closed principle and you're introducing view concerns into the domain.) By hiding your domain objects from the view, you give yourself a clean space to perform use-case-specific validation before changes get made to your domain object. The domain object may have its own set of validators, but they can be strictly domain-appropriate, without view-specific issues creeping in.

Answer 3

You should not inherit from Person or use composition to have Person on PersonViewModel. Instead you should add the properties that you need to the PersonViewModel and map between them. Tools like AutoMapper (http://www.codeplex.com/AutoMapper) make this dead simple.

You should not expose your domain model directly to the View for a number of reasons, including security (under-posting and over-posting).

Answer 4

I am thinking a partial class may be better here. keep the generated Person class and add anothe partial Person class with the extra stuff in it.

Answer 5

On the one hand:

What if you did this?

Person p = new PersonViewModel { //init some properties };

Would p do everything you'd expect a Person to do? If it would, then sure, use inheritance. If it would have some peculiarities related to the fact that it's really a PersonViewModel, and not a Person, then use composition.

On the other hand:

My inclination is to use inheritance largely as a way of avoiding lots of duplicated code. Since you're only inheriting from one parent to one child (rather than to many children), you're not avoiding a lot of duplicated code in the first place. So it's probably not worth it to use inheritance.

Answer 6

One trick I've used, not specifically with ASP.NET MVC but with a similar use case, is to create a class that contains specifically those items specific to the ViewModel class (i.e., those that are not just tunneling through to the person class), and supplying an extension property on the Person class to allow access to the extended properties. This isn't strictly a ViewModel in the classic sense of the word, but I feel it allows for much of the same functionality without introducing awkward inheritance or code duplication with tunneled properties.

Here's a quick example of what I'm talking about:

public static class PersonExtensions {
   public PersonViewData ViewData(this Person p) {
       return new PersonViewData(p);
   }
}

public class PersonViewData {
     public PersonViewData(Person p) {
         this._person = p;
     }

     private Person p;

     public string FormattedPhoneNumber {
         get { return p.PhoneNumber.ToPrettyString(); // or whatever }
     }
}