How to Simplify this Code Kludge

Question

Input:

Id, PartId, Name
1, 1, Head
1, 2, body
1, 3, Tail
2, 1, Head
2, 2, Leg

Output Display:

- Head, Body, Tail [Delete(1)]
- Head, Leg [Delete(2)]

My Code:

<ol>
<% 
    int prev = -1;
    foreach (var item in t)
    { 
        if(prev != item.ResponseId){
            if (prev != -1)
            {%>
                <%= Html.ActionLink("[replacethis]", "RemoveResponse", new { id = item.ResponseId })
                .Replace("[replacethis]", "<img src=\"../../Content/images/delete_icon.gif\" class=\"borderlessImage\" title=\"Remove\"/>")%>      
                </li>  
            <%} %>
            <li>
            <% }
        else {
            %>, <%
        } %>

      <%= Html.Encode(item.ResponsePartValue) %>   

    <% prev = item.ResponseId;
    }  %>

     <%= Html.ActionLink("[replacethis]", "RemoveResponse", new { id = prev })
                .Replace("[replacethis]", "<img src=\"../../Content/images/delete_icon.gif\" class=\"borderlessImage\" title=\"Remove\"/>")%>      
                </li> 
</ol>

Questions:

1. What are the ways to refactor this?
2. Any MVC tricks I am missing?

Answer 1

Well, first of all you could create an HtmlHelper that renders image links for you, instead of generating the anchor tags first and then replacing their content with an image.

Take a look at here.

Also you don't have to use <%= every time you need to ouput some text. If you already have opening code blocks (ie <%), then you can just use Response.Write method to output what you want. In cases like yours, that'll most likely look better than %> <%=.

Though, I admit I don't exactly know what you are listing here and how you want it to display. But following your algorithm, I guess this is what I would have done :

<ol>
<% 
    int prev = -1;
    foreach (var item in t) { 
        if(prev != item.ResponseId) {
            if (prev != -1) {
                Response.Write(Html.ImageLink("../../Content/images/delete_icon.gif", "RemoveResponse", new { id = item.ResponseId, @class ="borderlessImage", title = "Remove" }) + "</li>");
            }
            Response.Write("<li>");
        }
        else {
            Response.Write(", ");
        } 
        prev = item.ResponseId; 
        Response.Write(Html.Encode(item.ResponsePartValue));
    }  %>

     <%= Html.ImageLink("../../Content/images/delete_icon.gif", "RemoveResponse", new { id = prev, @class ="borderlessImage", title = "Remove" }) %>
     </li> 
</ol>

Answer 2

I would put everything into a dictionary, it would make your logic more logical :-)

Something like:

IDictionary<int, List<Part>> partsDictionary = new Dictionary<int, List<Part>>();

Where the int key is your id and then the value of type List would be your individual parts.

Then put the logic into a HtmlHelper extension.

E.g. (Although I don't know what you're doing, view code doesn't match you db model at the top. This should give you an idea)

public static string PartsList(this HtmlHelper html, IDictionary<int, List<Part>> partsDictionary)
{
    if (partsDictionary.Count == 0)
        return "";

    StringBuilder toReturn = new StringBuilder("<ol>");
    foreach (KeyValuePair<int, List<Part>> kvp in Model.PartsDictionary)
    {
        toReturn.Append("<li>");

        //Individual part links
        IList<string> partsLinks = new List<string>();
        foreach (Part part in kvp.Value)
            partsLinks.Add(html.ActionLink(part.PartName, "ActionName", new { @id = part.Id }));

        toReturn.Append(string.Join(", ", partsLinks.ToArray()));

        //Part category(?) link
        toReturn.Append(html.ActionLink("Delete", "ActionName", new { @id = kvp.Key }));

        toReturn.Append("</li>");
    }

    toReturn.Append("</ol>");
    return toReturn.ToString();
}

Or something like that ;-)

HTHs
Charles