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 a drop-down to populate a list of cities. Everything works fine, but I would like to know better ways of doing this. Also, please let me know if it is possible to create the same drop down using <Select> instead of HTML helpers.

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

I've used entity framework database first approach to get the data back from database. Please address improvements that need to be done.

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

I'd also like to know how the lamdba expression works in this scenario:

//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

2 Answers 2

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
  • For the DTOs, namespace them and keep them at that level. It's hard to add more here without more context around what you are doing.

View

  • In your view, you should not be doing the code inside of your @. This should all take place at the controller level or lower. Also, you should have a strongly typed ViewModel that contains exactly what the view needs.

Controller

  • Too much logic here. That query should be contained inside of a service/repository and you should simply call _cityService.GetAllCities() in the controller.
  • After that's done, you can create a class to map from the cities to whatever you need to have in your viewmodel.
  • Remove the ViewBag use, create a strongly typed ViewModel that you pass to your view.
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.