ASP controller for tracking equipment

Question

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

Answer 1

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.