Common method among multiple controllers

Question

I have a common method among multiple controllers, not all. Is correct to put the method in a controller base and all other controllers inherit it?

public class BaseController : Controller
{
    public IEnumerable<SelectListItem> GetStatus()
    {
        IList<SelectListItem> status = new List<SelectListItem>();

        status.Add(new SelectListItem() { Text = "Select", Value = "" });

        Enum.GetValues(typeof(Status)).Cast<Status>().ToList().Select(x => new SelectListItem()
        {
            Text = x.ToString(),
            Value = ((byte)x).ToString()
        }).ToList().ForEach(status.Add);

        return status;
    }
}

public class DownloadController : BaseController
{
    public ActionResult New()
    {
        NewViewModel newViewModel = new NewViewModel();

        newViewModel.Status = GetStatus();

        return View(newViewModel);
    }
}

If you have shared ActionResults I would say doing it in a base class is ok. If you want to share a property or object like a list of statuses, put it in to a Common class of some sort and access it that way. — Nick Bork, Jan 06 '12 at 17:46
Yep, I do it either. It may depend on what you are doing with that though. — tugberk, Jan 06 '12 at 17:49

score 2 · Answer 1 · answered Jan 06 '12 at 17:48

2

That seems right, you could probably make it static and protected.

answered Jan 06 '12 at 17:48

Cade Roux

88,164
40
182
265

I catch something here. I understand what `protected` brings to that but what would `static` be useful for here? – tugberk Jan 06 '12 at 17:50
static means you can access the property without creating an instance of the object. – Nick Bork Jan 06 '12 at 17:58
1

@tugberk Because your method doesn't appear to use any member data, it is effectively static - it does not depend on an instance, but only upon the parameters and other static data (in your acse, the type system surrounding the Status class/enum) – Cade Roux Jan 06 '12 at 18:28
@tugberk Refactoring to static and seeing that it depends entirely on another type and nothing to do with the controller at all then also shows you that perhaps it shouldn't be part of the controller as well. – Cade Roux Jan 06 '12 at 18:30

score 2 · Answer 2 · edited May 23 '17 at 12:33

I would actually take a different approach. I use some custom HTML Helpers for this, similar to the following:

http://blogs.msdn.com/b/stuartleeks/archive/2010/05/21/asp-net-mvc-creating-a-dropdownlist-helper-for-enums.aspx

That way you can just use:

 <%: Html.EnumDropDownListFor(model => model.EnuProperty) %>

I prefer the answer submitted by Simon which allows you to use the Meta Description attribute to customzie the output for Enum names:

How do you create a dropdownlist from an enum in ASP.NET MVC?

score 2 · Accepted Answer · answered Jan 06 '12 at 18:06

Based on this line,

newViewModel.Status = GetStatus();

I would argue that the GetStatuses shouldn't be a method on a controller. Controllers should handle Http requests and return http responses. These responses can be files, views, json, etc... But it looks though as this is not how your using GetStatuses, and that it is not intended to be returned as an Http response. If this is indeed the case, it should go else where.

My MVC applications always have a service layer that serve up view models. So in my application, this service layer would server up the Statuses.

score 1 · Answer 4 · answered Jan 06 '12 at 17:45

I would favor composition over inheritance and encapsulate the code in another object, then inject the object into the controller. especially in this instance.

and in this case setting up the rendering of the Enum list may even be better suited for a partial controller/view.

Common method among multiple controllers

4 Answers4