MVC binding list of values to checkboxlist in an efficient way

Question

I have this code which generates and shows list of selectable items that user can post to controller in order to save into database.

Model looks like this:

public class DeliveryAddressSelectionModel()
{
public List<DeliveryAddress> DeliveryAddresses { get; set; } 
public List<int> SelectedAddressIDs { get; set; }   
}
public class DeliveryAddress()
{
public int ID { get; set; }
public string Location { get; set; }
}

Controller looks like

[HttpGet]
public ActionResult Edit()
{
    var model = new DeliveryAddressSelectionModel();  
    model.DeliveryAddresses = AddressDetails.GetAvailable(); //Get from  EF
    model.SelectedAddressIDs = new List<int>(); //Populate this list with Address IDs for pre-selection     
    return View(model);
}

[HttpPost]
public ActionResult Edit(DeliveryAddressSelectionModel model)
{
    //save the model.SelectedAddressIDs in DB
    return RedirectToAction("Index");
}

And finally view looks like this:

<form method="post">
    <ul>
        @foreach(var address in DeliveryAddresses)
        {
        <li>
            <input id="address@(address.ID)" type="checkbox" 
                name="SelectedAddressIDs"
                value="@address.ID"
                @(Model.SelectedAddressIDs.Contains(address.ID) ? "checked" : "")
                />                  
            <label for="address@(address.ID)">@address.Location</label>
        </li>       
        }
    <ul>
    <input type="submit" value="Save Address"/>
</form>

This code works, but as I am learning MVC, and I would like to have suggestions. What areas in this code could have been done better, or what is best practice in regards with this kind of model population and posting? Could I use a @Html.CheckBoxFor() helper or this is ok?

Answer 1

In my opinion, your view is doing too much work. This is happening because your model is missing a property.

If you add an IsSelected property to your model, I think we can simplify the view a bit.

public class DeliveryAddress()
{
    public int ID { get; set; }
    public string Location { get; set; }
    public bool IsSelected { get; set; }
}

Now this single model contains all the information you need to bind to the view. You no longer even need the DeliveryAddressSelectionModel. The controller would look something like this (untested, I'm typing in the browser):

[HttpGet]
public ActionResult Edit()
{
    var selectedAddressIDs = new List<int>() { 1, 2, 4 };

    List<DeliveryAddress> model = AddressDetails.GetAvailable();

    foreach(var address in model.Where(a => selectedAddressIDs.Contains(a.ID)))
    {
        address.IsSelected = true;
    }

    return View(model);
}

Create a partial view for your DeliveryAddress.

@Model DeliveryAddress

@Html.CheckBoxFor(m => m.IsSelected)
@Html.LabelFor(m => m. Location)

Then, from your main view, render the partial view.

@Model System.Collections.Generic.List<DeliveryAddress>

<form method="post">
    <ul>
        @foreach(var address in Model)
        {
          <li>
            @Html.RenderPartial("DeliveryAddress", address)    
          <li />
        }
    <ul />
    <input type="submit" value="Save Address"/>
</form>