How to avoid ViewBag (or ViewData) in favor of a Model?

Question

This is a very simple example, but it should be enough to demonstrate my issue. I need to pass a model to my view that the user will update, but the view also needs some other data to create a dropdownlist or to provide other information.

Based on my code below, I want to avoid use of ViewBag/ViewData, so do I somehow combine QuestionList and PasswordLength (thrown in for a superfluous example scenario) into the ChangeSecurityQuestionModel or create a new ViewModel or some other object?

[Authorize]
public ActionResult ChangeSecurityQuestion() {
  var user = Membership.GetUser();
  if (user != null) {
    var model = new ChangeSecurityQuestionModel() {
      PasswordQuestion = user.PasswordQuestion
    };
    ViewBag.QuestionList = new SelectList(membershipRepository.GetSecurityQuestionList(), "Question", "Question");
    ViewBag.PasswordLength = MembershipService.MinPasswordLength;
    return View(model);
  }

  // user not found
  return RedirectToAction("Index");
}

[Authorize]
[HttpPost]
public ActionResult ChangeSecurityQuestion(ChangeSecurityQuestionModel model) {
  if (ModelState.IsValid) {
    var user = Membership.GetUser();
    if (user != null) {
      if (user.ChangePasswordQuestionAndAnswer(model.Password, model.PasswordQuestion, model.PasswordAnswer)) {
        return View("ChangeQuestionSuccess");
      } else {
        ModelState.AddModelError("", "The password is incorrect.");
      }
    }
  }

  // If we got this far, something failed, redisplay form
  ViewBag.QuestionList = new SelectList(membershipRepository.GetSecurityQuestionList(), "Question", "Question");
  ViewBag.PasswordLength = MembershipService.MinPasswordLength;
  return View(model);
}

Bluebonnet

This seems to scream of the DTO pattern – Uncial

Sorry, I hate seeing this arse-about-face code! 'if (user == null) return RedirectToAction("Index"); //code for user existing' – Disenable

I tend to look at MVC Models more like ViewModels, so adding to ChangeSecurityQuestionModel would also be the approach I would take. – Scaleboard

Back when I developed in .NET is separated my View and Input models, so in this example I would have the "View Model" that included the PasswordQuestion, QuestionList, and PasswordLength... then I would have an "Input Model" that would not include these pieces of information, just the inputs... Of course I also used ajax posts heavily which lended themselves to this as I didn't have to maintain state between posts (ie, repopulating the view models). – Ivan

Answer 1

Why not put QuestionList and PasswordLength in your ChangeSecurityQuestionModel

var model = new ChangeSecurityQuestionModel() {
      PasswordQuestion = user.PasswordQuestion,
      QuestionList = new SelectList(membershipRepository.GetSecurityQuestionList(), "Question", "Question"),
      PasswordLength = MembershipService.MinPasswordLength;
    };

Acotyledon

That seemed to be the obvious answer, but I was unsure if that was an appropriate place since the other properties in the model are related to and updated by the user. Thanks. – Bluebonnet

Answer 2

You could add the QuestionList and PasswordLength properties to your ChangeSecurityQuestionModel view model. And then:

[Authorize]
public ActionResult ChangeSecurityQuestion() {
    var user = Membership.GetUser();
    if (user != null) {
        var model = new ChangeSecurityQuestionModel() {
            PasswordQuestion = user.PasswordQuestion,
            QuestionList = new SelectList(membershipRepository.GetSecurityQuestionList(), "Question", "Question"),
            PasswordLength = MembershipService.MinPasswordLength
        };
        return View(model);
    }
    // user not found
    return RedirectToAction("Index");
}

[Authorize]
[HttpPost]
public ActionResult ChangeSecurityQuestion(ChangeSecurityQuestionModel model) {
    if (ModelState.IsValid) {
        var user = Membership.GetUser();
        if (user != null) {
            if (user.ChangePasswordQuestionAndAnswer(model.Password, model.PasswordQuestion, model.PasswordAnswer)) {
                return View("ChangeQuestionSuccess");
            } else {
                ModelState.AddModelError("", "The password is incorrect.");
            }
        }
    }
    // If we got this far, something failed, redisplay form
    model.QuestionList = new SelectList(membershipRepository.GetSecurityQuestionList(), "Question", "Question");
    model.PasswordLength = MembershipService.MinPasswordLength;
    return View(model);
}

Guilder

If this is acceptable to your sensibilities, that is all I wanted to know. Also, ChangePasswordQuestionAndAnswer in this code is a method, not a model, but I understand what you were saying. – Bluebonnet

Thanks again, Darin. I needed the reassurance that this was a good way to do it. Sorry, though, I had to accept dove's answer since it's essentially the same and he answered first. – Bluebonnet

Answer 3

One alternative to the recurring "do-I-use ViewState or keep adding to a Model" problem is to create extension methods for HtmlHelper:

public static class HtmlExtensions
{
     public static MvcHtmlString SecurityQuestionDropDown(this HtmlHelper helper)
     {
          return helper.DropDownList(....,new SelectList(membershipRepository.GetSecurityQuestionList(), "Question", "Question"));
     }
}

Contrasty

Ah, that's an interesting idea. – Bluebonnet

+1 - html helpers that can be reused for targetted problems such as this really do keep model polution to a minimum – Versify

does this not assume you have a static membershipRepository? – Acotyledon

@Acotyledon Just use the same setup as the controller, unless it's http context dependent – Contrasty