10
\$\begingroup\$

This was originally posted here. I'm hoping Code Review will be a little more helpful in giving me a concrete direction.

I've been working on a project based off of this tutorial.

Unfortunately, the controller classes in this are quite dense and have terrible separation of concerns. I should have realized this originally but, I ran with it and now am stuck dealing with the terrible design from this tutorial with my added features/bad decisions. I've been stuck and overthinking this.

In my case, I have 3 controllers: Hardware, Employee, Project. They access the database, filter the data and pass them along to the view. I also create certain UI components in the controller (see SelectList in the Create function below).

I want to break down these controllers into smaller, reusable parts. Filtering for example, could be re-used by different models but I can't quite abstract this in my head. There's a lot going on in most controller. Here's one of them for example:

public class HardwareController : Controller
{
    private LATTContext db = new LATTContext();

    // GET: Hardware
    public ViewResult Index(string sortOrder, string currentFilter, string searchString, int? page)
    {
        ViewBag.CurrentSort = sortOrder;
        ViewBag.SerialNoParm = sortOrder == "serialNo" ? "serialNo_desc" : "serialNo";
        ViewBag.CabModelParm = sortOrder == "cabModel" ? "cabModel_desc" : "cabModel";
        ViewBag.PlatformParm = sortOrder == "plat" ? "plat_desc" : "plat";
        ViewBag.VendorParm = sortOrder == "vendor" ? "vendor_desc" : "vendor";
        ViewBag.CategoryParm = sortOrder == "category" ? "category_desc" : "category";
        ViewBag.EmployeeParm = sortOrder == "employee" ? "employee_desc" : "employee";

        if (searchString != null)
        {
            page = 1;
        }
        else
        {
            searchString = currentFilter;
        }

        ViewBag.CurrentFilter = searchString;

        var hardwares = from h in db.hardwares
                        select h;
        if (!String.IsNullOrEmpty(searchString))
        {
            hardwares = hardwares.Where(h => h.SerialNo.ToLower().Contains(searchString.ToLower()) ||
                h.model.Name.ToLower().Contains(searchString.ToLower()) ||
                h.platform.Name.ToLower().Contains(searchString.ToLower()) ||
                h.vendor.Name.ToLower().Contains(searchString.ToLower()) ||
                h.category.Type.ToLower().Contains(searchString.ToLower()));
        }
        switch (sortOrder)
        {
            case "serialNo":
                hardwares = hardwares.OrderBy(h => h.SerialNo);
                break;
            case "serialNo_desc":
                hardwares = hardwares.OrderByDescending(h => h.SerialNo);
                break;
            case "cabModel":
                hardwares = hardwares.OrderBy(h => h.model.Name);
                break;
            case "cabModel_desc":
                hardwares = hardwares.OrderByDescending(h => h.model.Name);
                break;
            case "plat":
                hardwares = hardwares.OrderBy(h => h.platform.Name);
                break;
            case "plat_desc":
                hardwares = hardwares.OrderByDescending(h => h.platform.Name);
                break;
            case "vendor":
                hardwares = hardwares.OrderBy(h => h.vendor.Name);
                break;
            case "vendor_desc":
                hardwares = hardwares.OrderByDescending(h => h.vendor.Name);
                break;
            case "category":
                hardwares = hardwares.OrderBy(h => h.category.Type);
                break;
            case "category_desc":
                hardwares = hardwares.OrderByDescending(h => h.category.Type);
                break;
            case "employee":
                hardwares = hardwares.OrderBy(h => h.employee.FirstName);
                break;
            case "employee_desc":
                hardwares = hardwares.OrderByDescending(h => h.employee.FirstName);
                break;
            default:
                hardwares = hardwares.OrderBy(h => h.model.Name);
                break;
        }
        int pageSize = 10;
        int pageNumber = (page ?? 1);

        return View(hardwares.ToPagedList(pageNumber, pageSize));
    }

    // GET: Hardware/Details/5
    public ActionResult Details(int? id)
    {
        if (id == null)
        {
            return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
        }
        Hardware hardware = db.hardwares.Find(id);
        if (hardware == null)
        {
            return HttpNotFound();
        }
        return View(hardware);
    }

    // GET: Hardware/Create
    public ActionResult Create()
    {
        ViewBag.Category_ID = new SelectList(db.categories, "CategoryID", "Type");
        ViewBag.Employee_ID = new SelectList(db.employees, "EmployeeID", "FullName");
        ViewBag.Model_ID = new SelectList(db.models, "CabinetModelID", "Name");
        ViewBag.Platform_ID = new SelectList(db.platforms, "PlatformID", "Name");
        ViewBag.Vendor_ID = new SelectList(db.vendors, "VendorID", "Name");
        return View();
    }

    // POST: Hardware/Create
    // To protect from overposting attacks, please enable the specific properties you want to bind to, for 
    // more details see http://go.microsoft.com/fwlink/?LinkId=317598.
    [HttpPost]
    [ValidateAntiForgeryToken]
    public ActionResult Create([Bind(Include = "HardwareID,SerialNo,Comment,Platform_ID,Category_ID,Vendor_ID,Model_ID,Employee_ID")] Hardware hardware)
    {
        try
        {
            if (ModelState.IsValid)
            {
                db.hardwares.Add(hardware);
                db.SaveChanges();
                return RedirectToAction("Index");
            }
        }
        catch (DbUpdateException /*ex*/)
        {
            ModelState.AddModelError("", "Problem updating database. Please verify that Serial No is unique");
        }


        ViewBag.Category_ID = new SelectList(db.categories, "CategoryID", "Type", hardware.Category_ID);
        ViewBag.Employee_ID = new SelectList(db.employees, "EmployeeID", "FullName", hardware.Employee_ID);
        ViewBag.Model_ID = new SelectList(db.models, "CabinetModelID", "Name", hardware.Model_ID);
        ViewBag.Platform_ID = new SelectList(db.platforms, "PlatformID", "Name", hardware.Platform_ID);
        ViewBag.Vendor_ID = new SelectList(db.vendors, "VendorID", "Name", hardware.Vendor_ID);
        return View(hardware);
    }

    // GET: Hardware/Edit/5
    public ActionResult Edit(int? id)
    {
        if (id == null)
        {
            return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
        }
        Hardware hardware = db.hardwares.Find(id);
        if (hardware == null)
        {
            return HttpNotFound();
        }

        ViewBag.Category_ID = new SelectList(db.categories, "CategoryID", "Type", hardware.Category_ID);
        ViewBag.Employee_ID = new SelectList(db.employees, "EmployeeID", "FullName", hardware.Employee_ID);
        ViewBag.Model_ID = new SelectList(db.models, "CabinetModelID", "Name", hardware.Model_ID);
        ViewBag.Platform_ID = new SelectList(db.platforms, "PlatformID", "Name", hardware.Platform_ID);
        ViewBag.Vendor_ID = new SelectList(db.vendors, "VendorID", "Name", hardware.Vendor_ID);

        return View(hardware);
    }



    // POST: Hardware/Edit/5
    // To protect from overposting attacks, please enable the specific properties you want to bind to, for 
    // more details see http://go.microsoft.com/fwlink/?LinkId=317598.
    [HttpPost]
    [ValidateAntiForgeryToken]
    public ActionResult Edit([Bind(Include = "HardwareID,SerialNo,Comment,Platform_ID,Category_ID,Vendor_ID,Model_ID,Employee_ID")] Hardware hardware)
    {
        if (ModelState.IsValid)
        {
            db.Entry(hardware).State = EntityState.Modified;
            db.SaveChanges();
            if (Request.UrlReferrer.ToString().Contains("hardware/Edit"))
            {
                return RedirectToAction("Index");
            }
            else
            {
                return RedirectToAction("Index", "Project");
            }
        }
        ViewBag.Category_ID = new SelectList(db.categories, "CategoryID", "Type", hardware.Category_ID);
        ViewBag.Employee_ID = new SelectList(db.employees, "EmployeeID", "FirstName", hardware.Employee_ID);
        ViewBag.Model_ID = new SelectList(db.models, "CabinetModelID", "Name", hardware.Model_ID);
        ViewBag.Platform_ID = new SelectList(db.platforms, "PlatformID", "Name", hardware.Platform_ID);
        ViewBag.Vendor_ID = new SelectList(db.vendors, "VendorID", "Name", hardware.Vendor_ID);
        return View(hardware);
    }

    // GET: Hardware/Delete/5
    public ActionResult Delete(int? id)
    {
        if (id == null)
        {
            return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
        }
        Hardware hardware = db.hardwares.Find(id);
        if (hardware == null)
        {
            return HttpNotFound();
        }
        return View(hardware);
    }

    // POST: Hardware/Delete/5
    [HttpPost, ActionName("Delete")]
    [ValidateAntiForgeryToken]
    public ActionResult DeleteConfirmed(int id)
    {

        //TODO catch inner sql exception when hardware is assigned
        if (ModelState.IsValid)
        {
            Hardware hardware = db.hardwares.Find(id);
            db.hardwares.Remove(hardware);
            db.SaveChanges();

        }

        return RedirectToAction("Index");
    }

    protected override void Dispose(bool disposing)
    {
        if (disposing)
        {
            db.Dispose();
        }
        base.Dispose(disposing);
    }
}
\$\endgroup\$
0
7
\$\begingroup\$

Index

It would appear you primarily do three things here. Sort, Filter, and Page. There's not much to say about your paging, but the sorting and filtering logic clutter the action. Try creating an object that does the sorting and filtering for you. Something like this:

Note: I didn't include all the logic you need, however; I did include what I believe to be enough logic for you to get the idea.

public interface ISortProvider<T>
{
    IQueryable<T> Sort(IQueryable<T> source);
}

// Purpose: Hold sorting criteria (column, direction) and sort a query of hardware based on held sorting criteria.
public class HardwareSortProvider : IFilterProvider<Hardware>
{
    public string SortBy { get; set; }
    public bool IsDescending { get; set; } // Defaults to false, AKA Ascending.

    public IQueryable<Hardware> Sort(IQueryable<Hardware> source)
    {
        switch (SortBy)
        {
            case "serialNo":
                return IsDescending
                     ? source.OrderByDescending(h => h.SerialNo);
                     : source.OrderBy(h => h.SerialNo);
            // ...
            default:
                return source;
        }
    }
}

public ViewResult Index(string sortOrder, string currentFilter, string searchString, int? page)
{
    ViewBag.CurrentSort = sortOrder;
    // ...
    var hardwares = db.Hardwares; // Why mix query-style and extension-style? Just use one.

    //
    // SORT:
    //
    var sortProvider = new HardwareSortProvider();
    // TODO: Set SortBy and IsDescending accordingly.
    hardwares = sortProvider.Sort(hardwares);

    //
    // FILTER:
    //
    var filterProvider = new HardwareFilterProvider();
    // ...
    hardwares = filterProvider.Filter(hardwares);

    // Execute and Page the query of Hardwares.
    return View(hardwares.ToPagedList(pageNumber, pageSize));
}

With this idea there are a few things you can do:

  • Pass sort criteria in constructor.
  • Add the sort provider to the action's parameter list and change your view's logic so that the SortBy and IsDescending automatically model bind.
  • As alluded to in the Index action above, you can just as easily create a FilterProvider class/interface to do a similar effect.

Other Actions

The only other thing I see is that the following code is repeated in many of your actions.

ViewBag.Category_ID = new SelectList(db.categories, "CategoryID", "Type", hardware.Category_ID);
ViewBag.Employee_ID = new SelectList(db.employees, "EmployeeID", "FirstName", hardware.Employee_ID);
ViewBag.Model_ID = new SelectList(db.models, "CabinetModelID", "Name", hardware.Model_ID);
ViewBag.Platform_ID = new SelectList(db.platforms, "PlatformID", "Name", hardware.Platform_ID);
ViewBag.Vendor_ID = new SelectList(db.vendors, "VendorID", "Name", hardware.Vendor_ID);

I commonly create a method to call this. The only slight caveat to this is if you ever need to change one of the actions calling it (not that common, but it happens sometimes).

View

So to elaborate on what I said before about making the SortProvider a parameter to the action.

public ViewResult Index(SortProvider sortProvider, string currentFilter, string searchString, int? page)
{
    // ...
    sortProvider.Sort(hardwares);
}

Your links would change to something like this:

@Html.ActionLink("Serial No.", "Index", "Hardware", new RouteValueDictionary() { { "sortProvider.SortBy", "serialNo" }, { "sortProvider.IsDescending", SomeBooleanCondition.ToString() } }, null)

That way it will automatically map to the SortProvider's properties named "SortBy" and "IsDescending".

NOTE: This is only possible if the SortProvider has a default constructor (no parameters) otherwise model binding won't know how to instantiate the object.

\$\endgroup\$
11
  • \$\begingroup\$ Thank you for the answer. Do you think I should put those in the HardwareController.cs file for now? And as I abstract other features out the other Controllers, I can pool together things that are similar? \$\endgroup\$
    – J Hache
    Jun 22 '16 at 17:54
  • 2
    \$\begingroup\$ I would suggest giving them their own files. It's usually not a good idea having multiple classes in a single file. You should focus on the SRP (Single Responsibility Principal). If something relates to sorting a particular class create a SortProvider for that class, if it relates to filtering for a particular class (e.g. Hardware) create a filter provider for it, etc. \$\endgroup\$
    – Shelby115
    Jun 22 '16 at 18:01
  • \$\begingroup\$ I was going to do it later, but you're right, I might as well do it now. I have the traditional MVC structure i.e: Models have a folder, controllers do too, etc. I have a folder for the Interfaces. Should I group Concrete Implementations as a folder? Or is sorting and filtering considered a Service? I'm not too sure where HardwareSortProvider should be in my project structure. \$\endgroup\$
    – J Hache
    Jun 22 '16 at 18:06
  • 3
    \$\begingroup\$ I typically either have the interfaces in the same folder or a separate project entirely (depending the requirements and how lazy I'm being). Typically I place a folder inside the models folder named after that grouping of classes (e.g. FilterProviders, SortProviders, etc). Note: This isn't necessarily the best way, this is just how I do it. Not claiming to know the best way in this case \$\endgroup\$
    – Shelby115
    Jun 22 '16 at 18:11
  • 1
    \$\begingroup\$ I ended up using my original switch case, with the signature for the function being public IQueryable<Hardware> Sort(IQueryable<Hardware> hardwares, string sortBy). The interface's signature is IQueryable<T> Sort(IQueryable<T> source, string sortBy);. I did not have to modify my view. A simpler refactor, which achieves the same thing. \$\endgroup\$
    – J Hache
    Jun 28 '16 at 16:26

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.