5
\$\begingroup\$

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?

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ A hell of a solution ! \$\endgroup\$ – Flame_Phoenix Dec 19 '14 at 16:52
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.