HtmlHelper building a dropdownlist, using ServiceLocator : code smell ?

Question

Is it a code smell to have to following pattern, given the following code (highly simplified to get straight to the point) ?

The models :

class Product
{
    public int Id { get; set; }
    public string Name { get; set; }
    public Category Cat { get; set; }
}

class Category
{
    public int Id { get; set; }
    public string Label { get; set; }
}

The view to edit a Product :

<% =Html.EditorFor( x => x.Name ) %>
<% =Html.EditorFor( x => x.Category ) %>

The EditorTemplate for Category

<% =Html.DropDownList<Category>() %>

The HtmlHelper method

public static MvcHtmlString DropDownList<TEntity>(this HtmlHelper helper)
    where TEntity : Entity
{
    var selectList = new SelectList(
        ServiceLocator.GetInstance<SomethingGivingMe<TEntity>>().GetAll(), 
        "Id", "Label");

    return SelectExtensions.DropDownList(helper, "List", selectList, null, null);
}

For information, the real implementation of the helper method takes some lambdas to get the DataTextField and DataValueField names, the selected value, etc.

The point that bothers me is using a servicelocator inside the HtmlHelper. I think I should have a AllCategories property in my Product model, but I would need to be populated in the controller every time I need it.

So I think the solution I'm using is more straightforward, as the helper method is generic (and so is the modelbinder, not included here). So I just have to create an EditorTemplate for each type that needs a DropDownList.

Any advice ?

Answer 1

advice: look at the asp.net mvc sample application from here: http://valueinjecter.codeplex.com/ good luck ;)

This is how ValueInjecter's Sample Application could get the dropdowns: (but it doesn't right now cuz I'm ok with the Resolve thing)

public class CountryToLookup : LoopValueInjection<Country, object>
{
    ICountryRepo _repo;

    public CountryToLookup(ICountryRepository repo)
    {
        _repo = repo;
    }

    protected override object SetValue(Country sourcePropertyValue)
    {
        var value = sourcePropertyValue ?? new Country();
        var countries = _repo.GetAll().ToArray();
        return
            countries.Select(
                o => new SelectListItem
                         {
                             Text = o.Name,
                             Value = o.Id.ToString(),
                             Selected = value.Id == o.Id
                         });
    }
}

Answer 2

IMHO I'd leave it the way it is, have the same thing in another project.

BUT the service location bothered me as well so for another project I made this part of an ActionFilter which scans a model, finds all the anticipated dropdowns and does a batch load into ViewData. Since the ServiceLocator or Repository/Context/whatever is already injected into the Controller you don't have to spread your service location all over the place.

public override void OnActionExecuted(ActionExecutedContext filterContext)
{
    foreach( var anticipated in SomeDetectionMethod() )
    {
          var selectList = new SelectList(
    ServiceLocator.GetInstance<SomethingGivingMe<TEntity>>().GetAll(), 
    "Id", "Label");

         ViewData["SelectList." + anticipated.Label/Name/Description"] = selectList;
    }
}

In the view you can then make a helper to load up those dropdowns via a custom editor template or other method.