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

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.