Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have created the below drop-down to populate a list of cities.

All works fine, but I would like to know the better ways of doing this please. Also, please let me know if it is possible to create the same drop down using <Select> instead of HTML helpers.

Below are my DTO classes. Please advise if I can make improvements in the controller and the view as well.

//ViewModel

public class LocationDTO
{
    public IEnumerable<CityDTO> Cities { get; set; }
    public LocationDTO()
    {
        this.Cities = new CityDTO[] { };
    }
}

public class CityDTO
{
    public string CityId { get; set; }
    public string CityName { get; set; }
}

Below is my Controller, and I've used entity framework database first approach to get the data back from database. Could you please address the improvements that needs to be done on my controller?

//Controller

Models.LocationDTO Loc = new Models.LocationDTO();
EF.LocationEntities locCtx = new EF.LocationEntities();

public Action Result Index() {
    using(locCtx) { 
        var locResults    = (from q in locCtx.usp_GetAllCities()
                       Select new Models.CityDTO {
                       CityId = q.Id, 
                       CityName = q.Name  }); 
        loc.Cities = locResults.ToList();
    }

    List<Models.CityDTO> citiesList = new List<Models.CityDTO>();
    Models.CityDTO city = new Models.CityDTO() { CityId = "-1", CityName = "Select City" };
    citiesList.Add(city);
    citiesList.AddRange(Loc.Cities.ToList());

    ViewBag.CitiesDropDown = citiesList;
    return view(loc);
}

Below is my View. I'd also like to know how the Lamdba expression works in this scenario please.

//View

@{
    List<TestApp.Models.CityDTO> citiesList = ViewBag.CitiesDropDown;
    var cityItems = new SelectList(citiesList, "CityId", "CityName");
}
<div>
    Cities: @Html.DropDownListFor(x => x.Cities.SingleOrDefault().CityID, @cityItems)
</div>
share|improve this question

1 Answer 1

View Model

Looks fine to me, although I'd recommend that if all your DTO classes are to be appended with 'DTO' you instead put them inside a namespace named 'DTO' and leave the 'DTO' characters off the class names entirely.

Controller

Use var when the Right Hand Side (RHS) is obvious.

Also, I don't like the indentation in your LINQ statement. It makes it difficult to discern between LINQ and constructor.

There is no point in calling ToList so much. AddRange is going to enumerate the whole collection anyway, so you don't actually need any ToList calls at all.

var Loc = new Models.LocationDTO();
var locCtx = new EF.LocationEntities();

public Action Result Index() {
    using(locCtx) { 
        var locResults = (from q in locCtx.usp_GetAllCities()
                             Select new Models.CityDTO 
                             {
                                 CityId = q.Id, 
                                 CityName = q.Name
                             }); 
        loc.Cities = locResults;
    }

    var citiesList = new List<Models.CityDTO>();
    var city = new Models.CityDTO()
    {
        CityId = "-1", 
        CityName = "Select City" 
    };
    citiesList.Add(city);
    citiesList.AddRange(Loc.Cities);

    ViewBag.CitiesDropDown = citiesList;
    return view(loc);
}

View

You make a call to Cities.SingleOrDefault(). SingleOrDefault() can return a null value if your City class' default operator returns null (which is the err default behaviour). If this happens, accessing CityId will fail.

//View

@{
    List<TestApp.Models.CityDTO> citiesList = ViewBag.CitiesDropDown;
    var cityItems = new SelectList(citiesList, "CityId", "CityName");
}
<div>
    Cities: @Html.DropDownListFor(x => x.Cities.SingleOrDefault().CityID, @cityItems)
</div>
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.