Replicated nested repeater behaviour in MVC

Question

Today I decided to give MVC a go, and although I really like the idea, I found it fairly difficult to transition from ASP.NET and grasp some basic concepts, like using foreach instead of nested repeaters.

It took me good few hours to come up with this solution, but it doesn't seem quite right. Could someone please explain what's wrong with this code, and what the right way to do it is. Here is my solution:

Essentially it's a survey that consists of several questions, each of which has several answers. I have tables in db, which are represented as strongly typed entities. The controller looks like this:

public ActionResult Details(int id)
{
    return View(new Models.Entities().Questions.Where(r => r.PROMId == id));
}

and corresponding view like this:

<% foreach (var question in Model) { %>

    <h3>Question <%: Array.IndexOf(Model.ToArray(), question) + 1 %></h3>
    <p><%: question.QuestionPart1 %></p>
    <p><%: question.QuestionPart2 %></p>
    <% var answers = new Surveys_MVC.Models.Entities().Answers.Where(r => r.QuestionId == question.QuestionId); %>
    <% foreach (var answer in answers) { %>
        <input type="radio" /><%: answer.Text %>
    <% } %>

<% } %>

All feedback appreciated.

Answer 1

As far as using for loops for the nested repeater behavior, I think that's the best way to do this in MVC. But I would suggest you use dedicated ViewModels.

ViewModel:

public class RadioQuestionListViewModel
{
    public IEnumerable<RadioQuestionViewModel> Questions {get;set;}
}

public class RadioQuestionViewModel
{
    public int QuestionNumber {get;set;}
    public string InputName {get;set;}
    public string QuestionPart1 {get;set;}
    public string QuestionPart2 {get;set;}
    public IEnumerable<RadioAnswerViewModel> PossibleAnswers {get;set;}
}
public class RadioAnswerViewModel
{
    public int AnswerId {get;set;}
    public string Text {get;set;}
}

Controller:

public ActionResult Details(int id)
{
    var model = GetRadioQuestionListModelById(id);
    return View(model);
}

View:

<% foreach (var question in Model) { %>

    <h3>Question <%: question.QuestionNumber %></h3>
    <p><%: question.QuestionPart1 %></p>
    <p><%: question.QuestionPart2 %></p>
    <% foreach (var answer in question.PossibleAnswers) { %>
        <%: Html.RadioButton(question.InputName, answer.AnswerId) %>
        <%: answer.Text %>
    <% } %>
<% } %>

This approach has a few advantages:

1. It prevents your view code from depending on your data access classes. The view code should only be responsible for deciding how the desired view model gets rendered to HTML.
2. It keeps non-display-related logic out of your view code. If you later decide to page your questions, and are now showing questions 11-20 instead of 1-whatever, you can use the exact same view, because the controller took care of figuring out the question numbers to display.
3. It makes it easier to avoid doing a Array.IndexOf(Model.ToArray(), question) and a database roundtrip inside a for loop, which can become pretty costly if you have more than a few questions on the page.

And of course your radio buttons need to have a input name and value associated with them, or you'll have no way to retrieve this information when the form is submitted. By making the controller decide how the input name gets generated, you make it more obvious how the Details method corresponds to your SaveAnswers method.

Here's a possible implementation of GetRadioQuestionListModelById:

public RadioQuestionListViewModel GetRadioQuestionListModelById(int id)
{
    // Make sure my context gets disposed as soon as I'm done with it.
    using(var context = new Models.Entities())
    {
        // Pull all the questions and answers out in a single round-trip
        var questions = context.Questions
            .Where(r => r.PROMId == id)
            .Select(r => new RadioQuestionViewModel
                {
                    QuestionPart1 = r.q.QuestionPart1,
                    QuestionPart2 = r.q.QuestionPart2,
                    PossibleAnswers = r.a.Select(
                        a => new RadioAnswerViewModel
                             {
                                AnswerId = a.AnswerId,
                                Text = a.Text
                             })
                })
            .ToList();
    }
    // Populate question number and name
    for(int i = 0; i < questions.Count; i++)
    {
        var q = questions[i];
        q.QuestionNumber = i;
        q.InputName = "Question_" + i;
    }
    return new RadioQuestionListViewModel{Questions = questions};
}

Answer 2

I don't know if it is better, but you can create a helper to do this for you:

public static void Repeater<T>(this HtmlHelper html, IEnumerable<T> items, string cssClass, string altCssClass, string cssLast, Action<T, string> render)
        {
            if (items == null)
                return;
            var i = 0;
            foreach (var item in items)
            {
                i++;
                if (i == items.Count())
                    render(item, cssLast);
                else
                    render(item, (i % 2 == 0) ? cssClass : altCssClass);
            }
        }

Then you can call it like so:

<%Html.Repeater(Model, "css", "altCss", "lastCss", (question, css) => { %>
    <h3>Question <%: Array.IndexOf(Model.ToArray(), question) + 1 %></h3>
    <p><%: question.QuestionPart1 %></p>
    <p><%: question.QuestionPart2 %></p>
    <% var answers = new Surveys_MVC.Models.Entities().Answers.Where(r => r.QuestionId == question.QuestionId); %>
    <% foreach (var answer in answers) { %>
        <input type="radio" /><%: answer.Text %>
    <% } %>

<% }); %>

This has a lot of power and the above is just a general example. You can read more here http://haacked.com/archive/2008/05/03/code-based-repeater-for-asp.net-mvc.aspx