Select list items in ViewModel fetched using a Repository

Question

Doing database access in ViewModel is generally considered a bad practice. However, when you have a <select> in your view, you need to populate it using the ViewBag or the ViewModel (I prefer the ViewModel). This leads to the following code:

public class NewUserViewModel
{
    public string Username {get;set;}
    public IEnumerable<SelectListItem> Cities {get;set;}
    public User User // Helper to retrieve user
    {
        get { return new User() { Username = this.Username; }}
        set { this.Username = value.Username; }
    }
}

We now have to populate the Cities twice:

public ActionResult New()
{
    var vm = new NewUserViewModel();
    vm.Cities = addressRepository.GetAllCities().Select(c => new SelectListItem() { Text = c.Name, Value = c.Id });
    return View(vm);
}

[HttpPost]
public ActionResult Create(NewUserViewModel vm)
{
    if(ModelState.IsValid)
    {
        userRepository.Add(vm.User);
        return RedirectToAction("Index");
    }
    else
    {
        vm.Cities = addressRepository.GetAllCities().Select(c => new SelectListItem() { Text = c.Name, Value = c.Id }); // Copy this line
        return View(vm);
    }
}

I came up with an approach to solve this problem:

public NewUserViewModel(AddressRepository addressRepository) {
    this.Cities = addressRepository.GetAllCities().Select(c => new SelectListItem() { Text = c.Name, Value = c.Id });
}

I overrode the DefaultModelBinder to inject the Repository into the ViewModel.

protected override object CreateModel(ControllerContext controllerContext, ModelBindingContext bindingContext, Type modelType)
{
    return DependencyResolver.Current.GetService(modelType);
}

Now my controller is free:

public ActionResult New()
{
    // Is this bad too?
    return View(DependencyResolver.Current.GetService<NewUserViewModel>());
}

public ActionResult Create(NewUserViewModel vm)
{
    if(ModelState.IsValid)
    {
        userRepository.Add(vm.User);
        return RedirectToAction("Index");
    }
    else
    {
        return View(vm);
    }
}

If I think about it, having all the cities listed is ViewModel's job, not the controller's. This has one immediate downside: it fetches the Cities even when the ModelState.IsValid. However, that is not my concern. Is this bad practice? I won't do any more database access than this.

Answer 1

Yes I think its bad practise because you have introduced both complexity and dependencies when it simply isn't necessary. Its going to make your unit testing more difficult than it needs to be and it isn't the intended purpose of the your view models.

Just create a private method in your controller that loads the data and populates the ListItems and call it where necessary. Agreed that it's not pretty but it will cause you less issues in the long run than what you are doing now.

public class AdminController : Controller
{
    ...ctor etc...

    public ActionResult New()
    {
        var vm = new NewUserViewModel();
        PopulateListItems(vm);
        return View(vm);
    }

    [HttpPost]
    public ActionResult Create(NewUserViewModel vm)
    {
        if (ModelState.IsValid)
        {
            userRepository.Add(vm.User);
            return RedirectToAction("Index");
        }
        else
        {
            PopulateListItems(vm);
            return View(vm);
        }
    }

    private static void PopulateListItems(NewUserViewModel vm)
    {
        vm.Cities = _addressRepository.GetAllCities().Select(c => new SelectListItem() { Text = c.Name, Value = c.Id });
    }
}