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.

• Flame_Phoenix: A hell of a solution !

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.