current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 8 down vote favorite

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?

share|improve this question
up vote 6 down vote accepted

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>
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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