Is this the correct way to save form values in MVC3?

Here's my code:

[HttpGet]
public ActionResult Register()
{
    RegisterViewModel model = new RegisterViewModel();
    using (CityRepository city = new CityRepository())
    {
        model.SelectCityList = new SelectList(city.FindAllCities().ToList(), "CityID", "CityName");
    }

    using (CountryRepository country = new CountryRepositor开发者_StackOverflow中文版y())
    {
        model.SelectCountryList = new SelectList(country.FindAllCountries().ToList(), "CountryID", "CountryName");
    }

    return View(model);
}

[HttpPost]
public ActionResult Register(RegisterViewModel model)
{
    if (ModelState.IsValid)
    {
        //Actually register the user here.
        RedirectToAction("Index", "Home");
    }            

    //Something went wrong, redisplay the form for correction.
    return View(model);
}

Is this the best approach or is there another better tested way? Keep in mind that my database tables/field names are nothing like what I declared in my models. I have to scrape the values from the ViewModel and put them into an entity framework generated class to persist the information.

Anything here that screams out at you as wrong?

I use that pattern and another pattern which looks like this (important part is the AutoMapper part):

[HttpPost]
public ActionResult Register(RegisterViewModel model)
{
    if (!ModelState.IsValid)
    {
        // repopulate any input or other items set in GET
        // prefer to do at top due to ^^^ is easy to overlook
        return View(model);
    }

    // if it's an edit, pull to new instance
    // from the database and use automapper to
    // map over the submitted values from model to instance
    // then update instance in database
    //
    // VALUE: useful if form only shows
    // some of the properties/fields of model
    // (otherwise, those not shown would be null/default)

    // if it's new, insert

    RedirectToAction("Index", "Home");
}

That's the pattern I generally use.

I prefer this pattern:
Controller:

 [HttpGet]
        public ActionResult Index()
        {
            var cities= (from m in db.cities select m);
            ViewBag.Cities= cities;

            var states = (from m in db.States select m);
            ViewBag.States = states;

            return View();
        }
        [HttpPost]
         public ActionResult Index(RegisterViewModel model)
             {
             if (ModelState.IsValid)
                { 
                  // Saving the data
                  return View("ActionName", model);
                }
        return View();
    }

View:

@Html.DropDownList("DDLCities",new SelectList(ViewBag.Cities, "CityId" , "CityName" ), new { @class = "className" })
@Html.DropDownList("DDLStates",new SelectList(ViewBag.States, "StateId" , "StateName" ), new { @class = "className" })

Advised changes to [HttpGet]:

[HttpGet]
public ActionResult Register()
{
    // Get
    var cities = new List<City>();
    var countries = new List<Country>();

    using (CityRepository city = new CityRepository())
    {
       cities = city.FindAllCities().ToList();
    }

    using (CountryRepository country = new CountryRepository())
    {
       counties = country.FindAllCountries().ToList();
    }

    // Map.
    var aggregatedObjects = new SomePOCO(cities, countries);
    var model = Mapper.Map<SomePOCO,RegisterViewModel>(aggregatedObjects );

    // Return
    return View(model);
}

Summary of changes:

• Layout your logic in such a way the controller's job makes sense. Get - Map - Return. Exactly the tasks (in order) for which a Controller is designed for.
• Use AutoMapper to do the heavy lifting of ViewModel creation for you.

Advised changes to your [HttpPost]:

[HttpPost]
public ActionResult Register(RegisterViewModel model)
{
    if (!ModelState.IsValid) 
      return View(model);

    try
    {
       var dbObj = Mapper.Map<RegisterViewModel,SomeDomainObj>(model);
       _repository.Save(dbObj);
       return RedirectToAction("Index");
    }
    catch (Exception exc)
    {
       if (exc is BusinessError)
          ModelState.AddModelError("SomeKey", ((BusinessError)exc).FriendlyError);
       else
          ModelState.AddModelError("SomeKey", Resources.Global.GenericErrorMessage);
    }

    return View(model);
}

Summary of changes:

• Try/catch. Always need to capture exceptions, whether they are domain exceptions or lower-level (database ones)
• Check ModelState validity first. As @Cymen says - do it first so you don't forget later
• Add exceptions to ModelState. Use custom exception classes for business errors with descriptive, resource-based messages. If the error is too low-level for the user (foreign key constraint, etc), show a generic message