Is it okay to put private methods in my controller or should I separate them out into some type of helper class with asp.net mvc?

Question

I have a controller that loads some dropdowns based on the user type. For example:

public ActionResult Index()
    {
      switch (SessionHelper.ViewLimit)
          {
            case "C":
              ViewData["CustDivision"] = LoadCustDivisions();
              ViewData["Customer"] = LoadCustomers();
              break;
            case "P":
              ViewData["Customer"] = LoadCustomers();
              ViewData["Employee"] = LoadEmployees();
              break;
            case "D":
              ViewData["Customer"] = LoadCustomers();
              ViewData["Division"] = LoadDivisions();
              break;
             default:
              return RedirectToAction("Logout", "Account");
          }
    return View()
    }

First of all, does the switch statement belong in the controller and second of all if so, where should I put LoadCustomers(), LoadDivisions(), LoadEmployees()?

Answer 1

I feel NO - private method in a controller creates more problem than they solve. Here are my reasons:

By the time you feel like creating a private method in a controller, you have identified a piece of code that is ether a bit "down and dirty" or repetitive. This is enough reason to create a separate helper class or move the code down the stack.

A helper class, even with just 1 method, is much easier to test and mock. Also it creates a stronger logical separation of concern. This makes it easier to deal with when debugging.

I also agree with tvanfosson on using a strategy pattern in aid of not reinventing the wheel and demonstrating a more mature understanding of software development.

But in actual fact, this is one of those situations where you can argue both ways for eternity. But it comes down to the level of craftsmanship you're aiming for, or more accurately, willing to settle for.

Answer 2

If they are only used in this controller, I would say leaving them private to the controller is okay. Once you find that you have a need for them elsewhere, then look to migrate them to your DAL or a helper class.

The larger question of your architecture -- using switch statements or strategy pattern, etc. -- is hard to answer from just this snippet. I'm not particularly offended by this switch statement, but you may want to have your SessionHelper return a strategy that will load the correct view data for you. In that case, the code for loading the view would go in the strategy class.

  DataStrategy strategy = SessionHelper.GetDataStrategy()
  if (strategy == null)
  {
       RedirectToAction("Logout","Account");
  }

  strategy.LoadViewData( ViewData );

  return View();

Answer 3

Because ASP.NET MVC favors convention over configuration any public methods on a class ending with Controller are assumed to be action methods. If they're private, they're not.

So it's completely OK to have private methods in a controller class.