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 followin 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 contructor and have it do the quereies there?

Answer 1

You can for sure replace this loop by using Select()

public ActionResult Index()
{
    IEnumerable<MaterialRequestModel> model= DB.MaterialRequest
        .Where(m => m.MaterialStatusId == MatStatus.A9Cancelled || m.MaterialStatusId == MatStatus.A8Complete)
        .Select(r => new MaterialRequestModel(r, DB.Employees.Find(r.ConcernedEmployeeId), DB.Employees.Find(r.OrderedByEmployeeId), DB.Offices.Find(r.OfficeId)));
    return View(query.ToList());
}  

or much cleaner by a separate method

public ActionResult Index()
{
    IEnumerable<MaterialRequestModel> model= DB.MaterialRequest
        .Where(m => m.MaterialStatusId == MatStatus.A9Cancelled || m.MaterialStatusId == MatStatus.A8Complete)
        .Select(r => CreateMaterialRequestModel(r));
    return View(query.ToList());
}  

private MaterialRequestModel CreateMaterialRequestModel(MaterialRequest materialRequest)
{
    return new MaterialRequestModel(r, DB.Employees.Find(r.ConcernedEmployeeId), DB.Employees.Find(r.OrderedByEmployeeId), DB.Offices.Find(r.OfficeId));
}  

which can be improved further by caching the employee and office in a dictionary.

private Dictionary<int, Employee> cachedEmployees = Dictionary<int, Employee>();
private Employee GetEmployeeById(Int id)
{
    Employee employee;
    if cachedEmployees.TryGetValue(id, out employee)
    {
        return employee;
    }
    employee = DB.Employees.Find(id);
    if(employee != null)
    {
        cachedEmployees[id] = employee;
    }
    return employee;
}  

in the same way the office cacheing can be done, which results CreateMaterialRequestModel()

private MaterialRequestModel CreateMaterialRequestModel(MaterialRequest materialRequest)
{
    return new MaterialRequestModel(r, GetEmployeeById(r.ConcernedEmployeeId), GetEmployeeById(r.OrderedByEmployeeId), GetOfficeById(r.OfficeId));
}  

there is only a small problem. Any changes to an employee or an office won't be reflected in the dictionaries.

Answer 2

Your Indentation is a little weird here

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);
        }

I would first fix the indentation

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);
}

Then I would look at your model variable, it's supposed to be a list so instead I would name it models making it plural and clear that it is a collection of something.