how can i avoid unmaintanable code in asp.net mvc views

Question

i am finding more and more my asp.net mvc views are starting to look like my old crappy asp code that i could never maintain or keep clean with integrated <% %> and html put together. below i have a code sammple of what i am talking about. is there any best practice or recommended way of avoiding this and keeping the views more maintable and readable.

            <td>
                <%
                    bool userRequiresApproval = false;
                    if (!string.IsNullOrEmpty(item.loginName))
                    {
                        MembershipUserCollection membership = (MembershipUserCollection)ViewData["UnapprovedUsers"];
                        if (membership != null && membership[item.loginName] != null)
                        {
                            userRequiresApproval = true;
                        }
                    }
                    bool isLoggedInUserAdmin = false;
                    if (ViewData.ContainsKey("isAdmin"))
                    {
                        isLoggedInUserAdmin = (bool)ViewData["isAdmin"];
                    }
                    if (isLoggedInUserAdmin && userRequiresApproval)
                    {%>
                        <%= Html.ActionLink("View", "Details", new { id=item.Mail_ID })%>, <%= Html.ActionLink("Delete", "GotoDeleteConfirmPage", new { id = item.Mail_ID })%>, <%= Html.ActionLink("Approve", "Approve", new { id = item.Mail_ID })%>
                    <%}
                    else if (isLoggedInUserAdmin)
                    {%>
                        <%= Html.ActionLink("View", "Details", new { id = item.Mail_ID })%>, <%= Html.ActionLink("Delete", "GotoDeleteConfirmPage", new { id = item.Mail_ID })%>
                    <%}
                    else
                    {%>
                        <%= Html.ActionLink("View", "Details", new { id = item.Mail_ID })%>
                    <%}%>
        </tr>
        <% } %>

Answer 1

I'll address the concrete question first and then the abstract:

Concrete

One possible route could be to create a suite of Html helpers or user controls which have some base logic to determine whether they should be visible. For example, your usage could be:

<td>
    Html.LinkList(", "
        ActionLinks.ViewDetails(item),
        ActionLinks.DeleteAndConfirm(item),
        ActionLinks.Approve(item))
</td>

Each action contains its own logic for determining whether it should be used (e.g. "I require admin rights") and if that action determines its own criteria is not met, just return string.Empty:

class ActionLinks
{
    public static string Approve(Item item)
    {
        if(ItemRequiresApproval(item) && CurrentUserIsAdmin())
        {
            return Html.ActionLink("Approve", "Approve", new { id = item.Mail_ID });
        }
        else
        {
            return string.Empty;
        }
    }

    private static bool ItemRequiresApproval(Item item)
    {
        //determine whether item requires approval
        //this could be further broken into a separate utilities class
    }

    private static bool CurrentUserIsAdmin()
    {
        //this should definitely go in a separate class dedicated to
        //handling membership and authorization
        //as well as figuring out who the current user is
    }
}

The LinkList would look something like this:

string LinkList(string delimiter, params string[] links)
{
    StringBuilder sb = new StringBuilder();
    foreach(string link in links)
    {
        if(!string.IsNullOrEmpty(link))
        {
            sb.Append(delimiter);
            sb.Append(link);
        }
    }
    return sb.ToString().Substring(delimiter.Length);
}

Abstract

The solution to your problem lies in remembering SRP (Single Responsibility Principle) and SOC (Separation of Concerns). In your current example, your View is a class. You have made that class responsible not only for the overall structure of the markup, but every minute detail of nearly your entire application! Your View should not know or care about admin rights or approval. Only the approval buttons should know about approval. Only elements which are admin-specific should know about admin rights. If you find yourself repeating certain kinds of checks (for example, "if admin show x, otherwise show y"), create some generic wrappers like an AdminPanel which will toggle itself on or off appropriately. To all other players, a given element merely is or isn't - and it's that element's responsibility to make that decision.

Answer 2

Strongly typing your views, with a base class containing things like IsAdmin, will help (or even easier, populate that data in the session if it's not controller/action-specific).

For example, your entire code above can be consolidated to:

<tr><td>
<%
  Response.Write(Html.ActionLink("View", "Details", new { id=item.Mail_ID });
  if (Model.IsLoggedInUserAdmin)
  {
    Response.Write(", " + Html.ActionLink("Delete", "GotoDeleteConfirmPage", new { id = item.Mail_ID }));
  }
  if (Model.UserRequiresApproval)
  {
    Response.Write(", " + Html.ActionLink("Approve", "Approve", new { id = item.Mail_ID }));
  }
%>
</td></tr>

Or you could put the code between the <% %> into a control and pass in the required vars to a RenderPartial, that way your view isn't being cluttered by a bunch of Response.Write code, and your original 30 lines is consolidated to 3 (td, renderpartial, /td). Either way you clarify your view tremendously.

Answer 3

Use NHAML view engine.
It will help you in so many things and will save a lot of typing.

Additionally (and mainly):

• Pull any logic into controller or Model
• Use typed Model instead of ViewData
• Use typed views
• Use extension methods
• Use partial views