Think you can make this code prettier?

Question

Here's a simple MVC View that displays all Areas in a DB and then lists all Carreras in each Area under the header.

<h2>Listado General de Carreras</h2>

    <% foreach (var Area in (List<string>)ViewData["Areas"])
       { %>

       <p><span class="titulo"><%: Area%></span></p>

       <% foreach (var carrera in Model)
          {
              if (carrera.Area.Nombre == Area)
              { %>    

                <p><%=Html.ActionLink(carrera.Nombre, "Details", new { id = carrera.ID })%></p>

            <% }

          }
       }%>

Do you think you can make this code a bit prettier/efficient?

Answer 1

I don't think you need to pass the Areas in the ViewData. Sort your Model by Area. Then you just need a single loop through Model and you can check if carrera.Area.Nombre is different from the last one you saw, and if so, print out the header for it.

Hopefully that makes sense, but let me know if you need a nudge with code.

Answer 2

Kind of hard to tell without knowing what exactly is Area. But if it is possible, I would move areas into the view model, and make each Area object have collection of carreras.

Also to make the action link prettier you can use T4MVC.

You could end up with code like this:

<h2>Listado General de Carreras</h2>

<% foreach (var area in Model.Areas) { %>

    <p><span class="titulo"><%:area %></span></p>

    <% foreach (var carrera in area) { %>
        <p><%:Html.ActionLink(carrera.Nombre, MVC.Carrera.Details(carrera.ID)) %></p>
    <% } %>

<% } %>

Answer 3

You could get rid of the if(crrera.Area.Nombre == Area) with Linq. From the look of it your Model is some type of IEnumerable so your inner loop would become

<% foreach var carrera in Model.Where(c => c.Area.Nombre == Area))
{ %>
    <p><%= Html.ActionLink(carrera.Nombre, "Details", new { id = carrera.ID })%></p>
<% } %>

This wouldn't really help with "efficiency" but it is definitely "prettier" IMHO

Couple of MVC 3 features would also help on the prettiness front. First the Razor syntax, and second the new ViewModel property is of type dynamic so you'd get rid of the nasty dictionary lookup on ViewData and its associated cast i.e. your outer loop would become

<% foreach (var Area in View.Areas)
} %>

You could achieve something similar by creating a view specific model and tacking the Areas property on to that model