Use of Model and View (MVC)

Question

I have an application using c# and MVC5, with RazorEngine.

My application manages requests (orders from clients) and I need to show them in tables. To achieve this I have a Controller (HistoryController), a View (Index) and a Model (MaterialRequestModel) which takes several parameters in the constructor:

 public MaterialRequestModel(MaterialRequest order, Employee aConcernedEmployee, Employee anOrderedbyEmployee, Office anOffice)

In my controller, HistoryController I have the following index method, which gets all requests completed or canceled:

public ActionResult Index()
{
    IQueryable<MaterialRequest> query = DB.MaterialRequest
                .Where(m => m.MaterialStatusId == MatStatus.A9Cancelled || m.MaterialStatusId == MatStatus.A8Complete);

    List<MaterialRequestModel> model= new List<MaterialRequestModel>();

    foreach (MaterialRequest req in query)
    {
        model.Add(new MaterialRequestModel(req, DB.Employees.Find(req.ConcernedEmployeeId), DB.Employees.Find(req.OrderedByEmployeeId), DB.Offices.Find(req.OfficeId)));
    }

    return View(model);
}

Now, I could simply pass the query to the the View, but that is not a good practice and the community (this one!) strongly suggests I use Models to avoid placing logic into the view.

However, I can't help to think that the way I am building my model sucks terribly bad, because I am iterating over a large set of results.

How can I improve this code and make it decent without the loop?

Additionally, should I pass everything as a parameter to my model, or should I just pass the DB object into the Models constructor and have it do the queries there?

Does your EF model not have navigation properties from `MaterialRequest` back to `Employees` and `Offices`? If so your query would be cleaner and faster (one db request instead of many). — D Stanley, Dec 19 '14 at 16:33
I personally would advise against passing the data context into your constructor — Sam I am says Reinstate Monica, Dec 19 '14 at 16:33
Also your model should not be tied to your EF enitites - let the controller or a repository map from the EF entities to your model. — D Stanley, Dec 19 '14 at 16:34

score 4 · Accepted Answer · answered Dec 19 '14 at 16:36

        IQueryable<MaterialRequest> query= DB.MaterialRequest
            .Where(m => m.MaterialStatusId == MatStatus.A9Cancelled || m.MaterialStatusId == MatStatus.A8Complete)
            .Select(m => new MaterialRequestModel(m, DB.Employees.Find(m.ConcernedEmployeeId), DB.Employees.Find(m.OrderedByEmployeeId), DB.Offices.Find(m.OfficeId))
            .ToList();

If you just don't want to see the loop, you can use a select statement. and if this is EF, you have the added benefit of not querying your DB in the loop, since it will translate that select statement into sql.

Also, if you want to go to the next level, you can use foreign key references instead of all the DB.Employees.Find(...) Here's the result of my googling for that

score 1 · Answer 2 · edited May 23 '17 at 12:23

1

I try to answer some of your questions :)

eliminating the loop:

Create a query like Sam I am suggested. This way you would get all the data with single query and you could eliminate the loop.

the model:

Personally I like POCOs a lot, because I looks cleaner to me. Thats why I would not pass the DB into my model.

edited May 23 '17 at 12:23

Community

1
1

answered Dec 19 '14 at 16:33

Oliver

1,225
10
19

Use of Model and View (MVC)

2 Answers2