Is it valid to let view models populate their own lists for dropdowns in mvc?

Question

I've struggled to find any discussion on this.

If you have a simple MVC page for updating a User, and you put a dropdown list on it of Country, you need to populate the dropdown list in the viewmodel. Let's call it List<Country> countries {get; set;}

If the page is posted, and there's an error like they forgot to put a User Name in, we redirect them to the page again to try again, at which point we also have to remember to repopulate the model's List<Country> before redirecting them back to the page.

Three ways to handle this that I know of:

• In the controller, make a call to the database, e.g. viewModel.Countries = _dataAccess.GetCountries()
• Store the list in the view bag and not in the model at all e.g. ViewBag.Countries = _dataAccess.GetCountries()
• Put it into the view model itself, so that List<Country> {get { return _dataAccess.GetCountries(); and thus inject in a DataAccess object.

The third approach is what I am considering because then you don't need to worry about populating/repopulating the fields in the controller. But I can't find any evidence via searching that this is an acceptable method, so I fear there is a drawback I am not considering.

The third method also has a big problem in that GetCountries() is async, and you can't call async methods in properties or even in constructors it seems.

Answer 1

I think this is the main difference between MVC and MVVM approaches.

In MVC you have the controller populates the ViewModel which is essentially just the a struct of the various data the view needs

In MVVM you don't have a controller, so all the logic goes in the ViewModel, which is now a 'proper' class with methods and everything.

If you are in the ASP MVC framework, then you are constrained by the fact that the view is a webpage running on a client machine. It can't really make calls back to the viewmodel generated server side. So you might as well do the logic in the controller.

There's no obvious harm in having the controller build a ViewModel, call its methods and return the results. But it's an extra step you don't need.

Say for example, you have the page with its server side populated list of countries, populated via viewModel.GetCountries()

But you also have a client side api call which returns the list of countries.

Would you choose in your controller to do

public class UserController
{
    //MVVM
    public Action GetUserFormWithCountryDropDown
    {
        return View(new ViewModel(dataAccess)); //vm has dataAccess.GetCountries in a property
    }

    MVC
    public Action GetUserFormWithCountryDropDown
    {
        var vm= new ViewModel();
        vm.Countries = dataAccess.GetCountries()
        return View(vm);
    }
}

and

public class CountryApiController
{
    //MVVM
    public List<Country> GetCountryDropDown
    {
        var vm = new ViewModel(dataAccess)
        return vm.Countries() //calls dataaccess
    }
    //MVC
    public List<Country> GetCountryDropDown
    {
        return dataAccess.GetCountries();
    }
}

It doesn't make sense from the API to use the MVVM style because its just using a single property from the VM.

Further, you can't bind your client side commands to the server side VM. Its instanciated and discarded on every HTTP call.

The downside of the MVVM approach becomes apparent when you have a complex viewmodel and only want a small part of it to perform an action. ie.

public class CountryApiController
{
    //MVVM
    public List<Country> GetCountrys
    {
        var dataAccess = new DataAccess(countries);
        var translator = new Translator(htmlContext.Local);
        var facialRecognition = new Recogniser(User.Picture);

        var vm = new UserViewModel(
            dataAccess,
            translator,
            facialRecognition,
            ... etc
            );

        return vm.Countries() //just return a list of countries, other logic not required
        //wait do i need to duplicate this for non user related country lists?
    }

}

Answer 2

If the page is posted, and there's an error like they forgot to put a User Name in, we redirect them to the page again to try again, at which point we also have to remember to repopulate the model's List before redirecting them back to the page.

As you mention, main problem is recalculating or refilling something in post method. Also, you already fill data in get method. So, I focus "How to fill VM without repeating yourself(less code)?" and "who is responsible to fill VM: itself or controller?". I do not even consider ViewBag because of it is not strong type. I prefer VM instead of VB for this case.

In MVC, controllers are responsible to fill VM. VM just cares type of data. It can't know there are European countries or countries all over the world. So each controller can fill VM different number of data. Thus, I don't recommend to get data in VM by using dataaccess. VM is not responsible for doing this in MVC. If you intend to create kind of properties on VM (such as EuropeanCountries, AsiaCountries ..) it will look like service layer class over entity layer. But it is ViewModel(VM).

I recommend to create private method(s) in controller. You can use this private method(s) in get and post methods. So, you achieve DRY and you can call this methods in post method. Also, every controller can have kind of filling methods when using same VM. One can fill European countires to VM's Countries property and another one can fill all countries to Countries property.

Sample usage in a controller by get, post and one private method is below.

Your controller methods =>

[HttpGet]
public ActionResult Create()
{
    TaskViewModel viewModel = new TaskViewModel();
    viewModel = initViewModel(viewModel);
    return View(viewModel);
}

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Create(TaskViewModel viewModel)
{
    try
    {
        if(ModelState.IsValid)
        {
            // ... to do 
        }
    }
    catch (Exception ex)
    {
        ModelState.AddModelError("", ex.GetProperExceptionMessage());
    }
    // when you do here like that, user country selection and/or other selection(s) will be kept. 'SelectedCountryId'
    viewModel = initViewModel(viewModel);
    return View(viewModel);
}

// by passing view model as parameter, you can keep user selection.
private TaskViewModel initViewModel(TaskViewModel viewModel)
{
    viewModel.Countries = _context.GetCountries();
    //.. others

    return viewModel;
}

Your model =>

public class TaskViewModel
{
    public IEnumerable<Country> Countries { get; set; }
    public int SelectedCountryId { get; set; }
    public string TaskDescription { get; set; }
}

Answer 3

Because the view model is recreated with each request, it can be a good idea to separate the "thing" that creates the options for your drop downs from your view models. It has the added benefit of making it even easier to added auto fill drop downs later, where you have a controller action that simply returns new options for the drop down in JSON format.

I like to keep entities out of my view models as much as possible to reduce coupling, and reflect the fact that a view model is a transient object created and disposed of in each request. The "data access" required to generate the options can deal with entities, but using IEnumerable<SelectListItem> objects to represent drop down lists is a good way to continue this decoupling between entity and view model.

For lack of a better term, you can call this new "drop down generating thing" the DropDownOptionFactory. This can be a plain old C# object that receives your data access object as a constructor argument. It will have methods to generate the lists of options:

public class DropDownOptionFactory
{
    private readonly DataAccess dataAccess;

    public DropDownOptionFactory(DataAccess dataAccess)
    {
        this.dataAccess = dataAccess;
    }

    public IEnumerable<SelectListItem> CreateCountryOptions(string defaultItemText = "Select Country", string defaultValue = "")
    {
        var options = new List<SelectListItem>()
        {
            new SelectListItem()
            {
                Text = defaultItemText,
                Value = defaultValue
            }
        };

        options.AddRange(from c in dataAccess.GetCountries()
                         orderby c.Name
                         select new SelectListItem()
                         {
                             Text = c.Name,
                             Value = c.CountryCode
                         });

        return options.AsReadOnly();
    }
}

Now your view model just needs a property for the drop down option factory:

public class AddEditUserProfileViewModel
{
    public DropDownOptionFactory DropDowns { get; set; }
    // ... other properties
}

public ActionResult Create()
{
    var model = new AddEditUserProfileViewModel()
    {
        Dropdowns = new DropDownOptionFactory(dataAccess)
    };

    return View(model);
}

Repopulating the drop downs on post-back requires setting a single property:

public ActionResult Create(AddEditUserProfileViewModel model)
{
    if (ModelState.IsValid)
    {
        // Save it
        return RedirectToAction("Details", { id = ... });
    }

    // Validations failed, repopulate drop downs
    model.Dropdowns = new DropDownOptionFactory(dataAccess);

    return View(model);
}

Same principal applies when "editing" a resource as well. Lastly, you have a place for this drop down logic to live on its own, so implementing an auto fill feature just means creating another controller action to return the options:

public ActionResult CountriesJson()
{
    var dropdowns = new DropDownOptionFactory(dataAccess);

    return JsonOptions(dropdowns.CreateCountryOptions());
}

private ActionResult JsonOptions(IEnumerable<SelectListItem> options)
{
    return Json(options.Select(o => new { text = options.Text, value = options.Value }));
}

Answer 4

Here's my current method for accomplishing what you're after. I like this in that there's JUST ONE controller method "per page".

// Here's the ViewModel, you can add validation via IValidatableObject if you like

public class CreateWidgetViewModel 
{
    public int SelectedDangleId { get; set; } // to be collected from user input via DropDownList

    [ReadOnly(true)] // prevents malicious user from posting their own collection thru Model Binder
    public IEnumerable<Dangle> DangleChoices { get; set; }

    public IEnumerable<SelectListItem> GetDangleChoices() =>
        SelectlistHelpers.Generate(AvailableExamTemplates.ToDictionary(k => k.id, v => v.name), SelectedExamTemplateId);
        // thats a helper method of mine that takes a Dict<T1, T2> and turns it into a collection of SelectListTems
}

// The controller action here is just ONE route w/ the method below, so you won't forget to populate the Dropdownlist on a POST.
// Its also nice that I control my ViewModel constructor, its never envoked through the model binder.

[Route("CreateWidget/{fooId:int}")]
[AcceptVerbs(HttpVerbs.Get | HttpVerbs.Post)]
public ActionResult CreateWidget(int fooId)
{
    var viewModel = new CreateWidgetViewModel();

    if (Request?.HttpMethod == "POST") 
    {
        // This binds the dropdown choice to SelectedDangleId
        // If your ViewModel implements IValidatableObject, it runs here and affects ModelState.IsValid appropriately
        TryUpdateModel(viewModel);  

        if (ModelState.IsValid) 
        {
            // all good? then do something w/ SelectedDangleId, and fooId from URL param, etc
            if (allGood)
                return Redirect($"/Widgets/{whatever}/");
        }
    }

    viewModel.DangleChoices = getDanglesFromDBaseOrMaybeCache();
    return View(vm);
}


// In my view, I bind the Dangle choices to a dropdown via:

@Html.DropDownListFor(model => model.SelectedDangleId, GetDangleChoices())

Answer 5

I use the Post-Redirect-Get Pattern (PRG). In the PRG pattern, a post always redirects to a GET even when ModelState is invalid. People mostly think of PRG as a way to avoid duplicate form submissions, but it is also useful to avoid having to "repopulate" view models.

With the PRG pattern, instead of displaying the view when the model state is invalid you redirect to the original GET page and the view model is populated as it normally would be if as if you were going to the GET for the first time.

So instead of this:

[HttpPost]
public ActionResult Edit(EditViewModel model)
{
    if(!ModelState.IsValid)
    {
        // Repopulate View Model
        model.Counties = GetCountriesForDropDown();

        return View(model);
    }
    // Do Post Stuff
    return RedirectToAction("Index");
}

Do this:

[HttpPost]   
public ActionResult Edit(EditViewModel model)
{
    if (!ModelState.IsValid)
    {
        return RedirectToAction("Edit");
    }
    
    // Do Post Stuff
    return RedirectToAction("Index");
}

But what about ModelState? Won't it be gone by the next request? What about displaying the errors? There are many ways to preserve ModelState so that it is available for the next request. I have seen solutions using SessionData and TempData. I prefer TempData. TempData allows data to exist for the current request and the next one, and then the data gets deleted. This seems well suited for the task at hand.

You could manually store ModelState data in TempData when there are post errors, but there are better ways. What's needed is an method to automatically store ModelState data in TempData when a ModelState is invalid. I use ActionFilters and Custom Attributes.

Get looks like this:

[HttpGet]
[ImportModelState]
public ActionResult Edit()
{
    var model = new EditViewModel();
    model.Counties = GetCountriesForDropDown();

    return View(model);
}

and POST looks like this:

[HttpPost]   
[ExportModelState]
public ActionResult Edit(EditViewModel model)
{
    if (!ModelState.IsValid)
    {
        return RedirectToAction("Edit");
    }
   // Do Post Stuff 
    return RedirectToAction("Index");
}

The key here is the attributes ImportModelState and ExportModelState on the GET and POST actions. There are several blog posts about this technique including this one here

Since this link might dissappear, I will include the relevant parts here:

public abstract class ModelStateTransfer : ActionFilterAttribute
{
    protected static readonly string Key = typeof(ModelStateTransfer).FullName;
}

public class ExportModelStateAttribute : ModelStateTransfer
{
    public override void OnActionExecuted(ActionExecutedContext filterContext)
    {
        //Only export when ModelState is not valid
        if (!filterContext.Controller.ViewData.ModelState.IsValid)
        {
            //Export if we are redirecting
            if ((filterContext.Result is RedirectResult) || (filterContext.Result is RedirectToRouteResult))
            {
                filterContext.Controller.TempData[Key] = filterContext.Controller.ViewData.ModelState;
            }
        }

        base.OnActionExecuted(filterContext);
    }
}

public class ImportModelStateAttribute : ModelStateTransfer
{
    public override void OnActionExecuted(ActionExecutedContext filterContext)
    {
        ModelStateDictionary modelState = filterContext.Controller.TempData[Key] as ModelStateDictionary;

        if (modelState != null)
        {
            //Only Import if we are viewing
            if (filterContext.Result is ViewResult)
            {
                filterContext.Controller.ViewData.ModelState.Merge(modelState);
            }
            else
            {
                //Otherwise remove it.
                filterContext.Controller.TempData.Remove(Key);
            }
        }

        base.OnActionExecuted(filterContext);
    }
}