Extra If Statements Or Repetative Code in C#

Question

Trying to figure out which makes more sense

<%foreach (var item in Model.items)
   {
%>
<tr>
    <td>
        <% if (!item.isMgmt)
           {  %>
        <a href="/MVC/AzureMail/Unfiled/<%:item.uName %>">
            <%:item.uName%></a>
        <% }
           else
           { %>
        <%:item.uName %>
        <% } %>
    </td>
</tr>
<% } %>

or

 <%foreach (var item in Model.items)
   {
%>
<tr>
    <td>
        <% if (!item.isMgmt)
           {  %>
        <a href="/MVC/AzureMail/Unfiled/<%:item.uName %>">
        <% } %>
              <%:item.uName%>
        <% if (!item.isMgmt)
           {  %>
              </a>
        <% } %>
    </td>
</tr>
<% } %>

Answer 1

I think the first one makes the most sense, but I'm not sure if there's really a right or wrong answer here. Just from a clarity standpoint the second one seems less readable.

Answer 2

in my opinion, the 1st example makes most sense as it keeps the href and closing tag within the same logical processing statement location. the second example get's repititious and could lead to errors if the code builds up and separates the two identical conditions as per #2.

anyway, just my thoughts

Answer 3

The first option. It seems more logical to have the all the related logic that creates the link in statement as opposed to the split in option 2.

Edit: I think most agree that option 1 is better. I am a proponent of HtmlHelpers (=cleaner views), so my additional suggestion would be that you create a Helper that wrap the logic you present.

Answer 4

3rd option; an extension method for conditional link.

public static string ConditionalHyperlink(this HtmlHelper helper, string url, string text, bool shouldLink){
 ...
}

This keeps your View much more readable.

<%= Html.ConditionalHyperlink("/MVC/AzureMail/Unfiled/" + item.Name, item.Name, item.isMgmt) %>

Answer 5

Really, you should consider having a view Model that wraps your domain model. Then your view model would contain a property that your view just renders and leave the logic of deciding the contents of that property to your controller. So, in your case you would have a property that returns a string called something like RenderName and set the value of that to be an HREF if isMgmt is true or a plain string if not. Then in your view you can just do this:

<%foreach (var item in Model.items)
   {
%>
<tr>
    <td>
        <%:item.RenderName %>
    </td>
</tr>
<% } %>

This way your View has nothing to do with making decisions and you don't have any ugly mark-up.

Answer 6

<%foreach (var item in Model.items)
{
%>
<tr>
    <td>
        <%: item.isMgmt ? item.uName : string.format("<a href=\"/MVC/AzureMail/Unfiled/{0}\"">{0}</a>, item.uName) %>
    </td>
</tr>
<% } %>

I'd still recommend using the html helpers, too