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 Username { 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.

Answer 2

In MVC, you want to avoid creating derived models because it locks your code into using them.

Imagine in the future if the View using the Base Model changes but not in relation to other views. Now all of your other views will have the access properties you added for the change to the base model. Or you could break the base model out, but then you need to update all of your views to use different models.

It's much easier to maintain an MVC site when each model is unique to it's view or partial view (or multiple views/partial views).

Secondly, there are ORM's and Micro Orm's that generate those Models for you from the database, it doesn't look like you are using one.

Even if you don't want to use EntityFramework 6, you can use a Micro ORM like Peta Poco which has T4 templates that can automatically generate all of your Poco's (Models).

As my sql database has tables that all have 3 common Sql Columns (Active, Created, Updated), all of my models have Properties for "Active, Created, and Updated" because I didn't write my models, Peta Poco Did.

Peta Poco generates all of the models as Partial Classes, so if I need to add properties to a model that are not database driven, I can just create a class with the same name in the same namespace and define it as partial and my additions will get added to the peta poco model.

Now let's say I want to add a derrived class to every Model generated by PetaPoco.. I can still do that. I would just create the new class, then create a partial class for each peta poco model and Derrive from my class in the partials.

Optionally I could modify the T4 Template for Peta Poco in my project to add my base class to the models it is generating.

Answer 3

This would basically need some analysis and would vary from scenario.

Let us say I have a list of virtual method/ property which is only resused in given amount and for rest I end up overloading the functionality, here I need to make a choice of whether I should allow individual class contain these or create a base class and inherit it which should be basis actual amount of code reusability.

If I have concrete functionality to be used by all or my most set of related classes,I would create a base class and inherit rather than changing individual classes in case of change to the common method.