Use viewbags for metadata or place them in a Viewmodel

Question

In the scenario below, I own a View that depends on some ViewBags to fill in options in selects html. When performing the POST and if something goes wrong in the registration, the user is redirected to the same View with fields filled in. In this method POST, I am currently replicating the creation of ViewBags. Is this the best approach? It is correct to put the meta data list (states, cities, neighborhoods, countries) in the ViewModel and load the selects for his sake?

public ActionResult Cadastrar()
{
       ViewBag.Estados =[...]; //List de selectListItem
       ViewBag.Cidades = [...];
       ViewBag.Bairros = [...];
       ViewBag.Paises = [...];

       return View();
}

[HttpPost
public ActionResult Cadastrar(CadastrarUsuarioViewModel model)
{
       ViewBag.Estados =[...]; //List de selectListItem
       ViewBag.Cidades = [...];
       ViewBag.Bairros = [...];
       ViewBag.Paises = [...];
       try
       {
           //DoSomething
           ExibirMensagemSucesso();
           return RedirectToAction("Index");
       }catch (Exception ex)
       {
            //Do Something
            return View(model);
       }       
}

Answer 1

This instruction order here is not good:

[HttpPost]
public ActionResult Cadastrar(CadastrarUsuarioViewModel model)
{
       ViewBag.Estados =[...]; //List de selectListItem
       ViewBag.Cidades = [...];
       ViewBag.Bairros = [...];
       ViewBag.Paises = [...];
       try
       {
           //DoSomething
           ExibirMensagemSucesso();
           return RedirectToAction("Index");
       }catch (Exception ex)
       {
            //Do Something
            return View(model);
       }       
}

First, that the catch of exceptions should be made in the event OnException of Controller, not in the Action.

Second, that the cargo of items must be made afterward of the main business rule of a Action decorated with [HttpPost], not before. This is simply because, if the rule works, you will not return the same View again, so there’s no need to carry the Viewbags again.

Third, that you do not validate the Viewmodel. This is done by ModelState through property IsValid. There is high chance that the application will perform unwanted logic there.

Change to the following:

[HttpPost]
public ActionResult Cadastrar(CadastrarUsuarioViewModel model)
{
       if (ModelState.IsValid) 
       {
           //DoSomething
           ExibirMensagemSucesso();
           return RedirectToAction("Index");
       }

       ViewBag.Estados =[...]; //List de selectListItem
       ViewBag.Cidades = [...];
       ViewBag.Bairros = [...];
       ViewBag.Paises = [...];
       return View();
}

Plus, it’s all right.