5

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.

  • 4
    Whatever you decide, I'd stay away from ViewBag. – Graham Jul 3 at 13:00
  • @Graham: Because ... – Robert Harvey Jul 3 at 15:03
  • I don't think there's enough information in your question to make it properly answerable. See my comments below Ewan's answer. – Robert Harvey Jul 3 at 15:38
  • @RobertHarvey: I think there is plenty of information to answer the question here. How do you architect the code so you can repopulate the dropdowns on post-back? It's a problem we all have with the ASP.NET MVC framework, and any web framework that is similar. – Greg Burghardt Jul 3 at 15:57
  • @RobertHarvey Because if you're utilizing ViewModels, you have no need for ViewBag, outside of weird edge cases. Everything needed for the View should be in the ViewModel. The dynamic-ness of ViewBag can burn you, its like you're separating the View from the ViewModel as if there were an application boundary, similar to the boundary between SQL and .NET, where you have no strong-typing guarantee of correctness. – Graham Jul 3 at 16:46
3

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?
    }

}
  • I don't understand this answer either. If you're going to go this route, wouldn't something like vm.RefreshCountries() be better? Surely the View Model already has access to the database. – Robert Harvey Jul 3 at 14:59
  • on your webpage you might have a javascript vm.RefreshCountries(). but would you have a serverside vm.RefreshCountries() when you are only populating the vm to return the result that one method? – Ewan Jul 3 at 15:17
  • Yes. The VM is responsible for all of the data that is presented to the View. – Robert Harvey Jul 3 at 15:19
  • For what it's worth, the endpoint in the VM that is providing countries can always be written in such a way that it goes back to the database each time data is compelled from it. – Robert Harvey Jul 3 at 15:21
  • 1
    The ugliness I really want to avoid the most is in the [POST] method of the form. You'd have if (ModelState.IsValid { many lines of code } else { DoSomeStuffIfInvalid(); WhenYou'reFinallyDoneThen(); viewModel.Countries = _dataAccess.GetCountries(); viewModel.xyzDropdown = _dataAccess.GetXyz(); etc. return View(viewModel); because it is very unclear that those properties need to be repopulated and you could easily forget to do it. You could make a RepopulateModel() method but if your controller has 20 pages you'd end up creating 20 such methods. – NibblyPig Jul 4 at 7:44
1

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; }
}

  • I don't understand your answer. Why is this the "best" way? – Robert Harvey Jul 3 at 14:54
  • I think what Engineert is saying is that you can't inject the datasource via constructor injection when the binder creates the ViewModel ? – Ewan Jul 3 at 15:32
  • I recently switched from the style in this answer to having just one controller method per 'page', which accepts GETS and POSTS. The ViewModel is always constructed in code, but if its a POST, I call TryUpdateModel(viewModel) and then operate on that data as needed. Now, I'm in control of my ViewModel constructor for both GETS and POSTS and my overall number of methods in a given controller drops by almost half. I'm really liking it so far. – Graham Jul 3 at 16:52
  • @RobertHarvey I tried to explain more clearly. – Engineert Jul 8 at 14:39
1

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 }));
}
0

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())

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.