-2

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.

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 example below).

I've been reading about Dependency Injection, the Repository pattern and the Unit of Work pattern. I'm confused a bit towards all 3 of these, but I tend to agree with "Repository is the new Singleton". If this is what EF6 does, I don't think it's a good idea to re-implement it. I think DI is definitely where I should be headed but I think my comprehension of it isn't quite there yet.

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 controllers:

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);
    }
}
  • I suggest you use the chat for discussions. Once you can formulate a more concrete question feel free to try again. – Frank Jun 21 '16 at 13:49
  • Thanks @Frank, I'll try that. Writing this up felt like a good exercise in getting my thoughts in order. – J Hache Jun 21 '16 at 13:55
  • That is actual one of the prime reasons of the existence of stackexchange. It forces you to create that order in your thoughts. Next time you'll probably already realize while writing the question, that you don't really know yet what to ask. – Frank Jun 21 '16 at 14:05
  • I'm currently going over the question again. I've been discussing with a couple coworkers and I have an idea of what I want but it's not quite fleshed out. The company mostly works on embedded systems and this is just a tool for managers that I created during my internship. This is far from the bread and butter, so I'm pretty alone in all this. – J Hache Jun 21 '16 at 14:06
0

If I were you I'd start by utilising the "M" in MVC and abandoning the ViewBag: start creating view models. If you begin by creating a model for your grid sorting, filtering and paging you'll start see where the common functionality is and isn't, i.e. what is specific to a given grid and what is general.

You don't have to go to the Repository pattern in order to separate out your data access from your UI concerns. Start by moving the data logic just to another class and look at establishing the interfaces/API between the UI and the business logic / data access.

Don't jump to DI just yet either. That might be a long term goal but start with separating your concerns.

  • Thanks, I'll be looking at that after lunch. It's a bit annoying that the MVC tutorials don't follow best practice. – J Hache Jun 21 '16 at 14:51
  • @JHache: Have you tied this so-called "best practice" to actual, tangible benefits in your particular project? If you haven't, it's not a "best practice;" it's just a "practice." Software developers change technologies and techniques like they change their underwear; the "best practice" of today will be so yesterday tomorrow. – Robert Harvey Jun 21 '16 at 14:58
  • @RobertHarvey I agree with you regarding best practices and how often they change, but if you compare my Index method to the one in the tutorial located here you'll see that they are quite alike. The best practice in this case would be KISS, which these methods aren't. It is my fault for following through and adapting these examples to my use case. I should have known better but here I am with these bad decisions. – J Hache Jun 21 '16 at 15:43

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.