Is it a bad practice using model classes in controller in mvc?

Question

Hi,

I wanted to compare with best practices when working with an ORM or database tables in asp.net mvc. One of the major questions I have is should I instantiate the model classes directly in controller..not query the database but just use the model class to store the values.

For e.g. If I am using entity framework as model...then is it a bad practice to use the entity class objects in the controller. There are times when it is just easier to directly use the database classes generated in the controller instead of creating ViewModels or even ViewData. We have a data access layer and a Business layer where all the querying and business logic is applied but although easier I don't like the idea of accessing the model in the controller but is it really a bad practice?

Answer 1

Yes, it is a bad practice, because of the problem of "over-posting".

For instance, consider an Entity model for a UserProfile:

  public class UserProfile
  {
    public string UserName { get; set; }
    public bool IsAdmin { get; set; }
    public string EmailAddress { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
  }

Your user profile page allows the user to Edit their FirstName, LastName, and EmailAddress.

An unscrupulous user could simply modify the form to post "IsAdmin" along with the other values. Because your Action is expecting an input of UserProfile, the IsAdmin value will be mapped as well, and eventually persisted.

Here is an excellent writeup about the perils of under and overposting.

I see nothing wrong with binding Entity models directly to your [HttpGet] methods, though.

Answer 2

Yes, by practice. You should keep controller as skinny as possible and separate each concerns.

Answer 3

Actually, it depends, maybe all you need is exactly show your model in UI. then I see no sense in wrapping it. But most of the times, even if you think you do not need to change anything in a model before showing it into view, you may need to do it in future. So the better way is to separate your exact model and future view data. It gives you more flexibility if you ned to change something (like change DB structure but view will remain the same)