Is it bad practice to have an inherit from a view model in Asp.net MVC?

Question

I have a situation where I have a bunch of common properties in a view model but have certain cases where I need to extend it. I created a couple of derrived view models, which I pas into views that expect the derived view Model. (I realize I could use a Polymorphic Binder but I'm happy with just passing in the derived view model for now.

The code looks something like this:

public class CommmonViewModel 
{
     public int? CommonA{ get; set; } 
     public int? CommonB{ get; set; } 
     public int? CommonC{ get; set; } 
}

public class SpecificViewModel1 : CommmonViewModel 
{
     public int? SpecificD{ get; set; }    
}

 public class SpecificViewModel2 : CommmonViewModel 
{
     public int? SpecificE{ get; set; }    
}

The code reviewer wants me to have three distinct view models and duplicate the code like so:

public class SpecificViewModel1 
{
     public int? CommonA{ get; set; } 
     public int? CommonB{ get; set; } 
     public int? CommonC{ get; set; } 

     public int? SpecificD{ get; set; }
}

public class SpecificViewModel2 
{
     public int? CommonA{ get; set; } 
     public int? CommonB{ get; set; } 
     public int? CommonC{ get; set; } 

     public int? SpecificE{ get; set; }
}

Now bearing in mind that the real life view models are quite a bit more complex than what I have shown here; I hate that the solution should be to duplicate the code as it feels like a pretty bad violation of DRY to me.

I know there are generally reasons why we would be careful with inheritance and favor composition but in his case inheritance works fine, especially since this part of the system is likely to change in future.

In short, is he right? is this a case where I should be happy with duplicating code? What are the advantages\disadvantages of inheriting in this scenario?

Thanks for your thoughts

Answer 1

If all of the properties from the underlying base class are used on your forms/derived classes, then in my opinion there is no issue. There could be security issues if you have view model properties which are not used on the form due to underposting/overposting attack.

Consider the following scenario. You have a User class in your domain:

public class User 
{
   public string Username { get; set; }
   public string Password { get; set; }
   public bool IsAdmin { get; set; }
}

Now suppose your login view model simply inherits from your domain model like this:

public class Login : User
{
   // Whatever overrides you have...
}

If the user is in some way aware of your domain model architecture he can add a hidden field on your login page which sets the IsAdmin property to true. This is what's called a overposting attack. This field will map to your model, because you are inheriting from User and possibly map to your domain model and change the user's role in the database.

Obviously this all depends on attacker (disgruntled employee maybe?) being aware of your architecture and your business logic not checking for such a scenario but it is possible under the right circumstances. I'm guessing that your code reviewer is maybe being too careful just in case, but in your case I think what you did is fine.