Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've come across the following piece of code in a Razor view

<table class="key-left">
    <tr>
        <td>Company</td>
        @foreach (Customer c in Model.SelectedInstallers)
        {
            <td>@c.Name</td>
        }
    </tr>
    <tr class="even">
        <td>Address</td>
        @foreach (Customer c in Model.SelectedInstallers)
        {
            <td>
                @c.Street<br />
                @c.City<br />
                @c.County<br />
                @c.Postcode
            </td>
        }
    </tr>
    <tr>
        <td>Contact Details</td>
        @foreach (Customer c in Model.SelectedInstallers)
        {
            <td>
                @c.Telephone<br />
                @c.Website
            </td>
        }
    </tr>
    <tr class="even">
        <td>Remove</td>
        @foreach (Customer c in Model.SelectedInstallers)
        {
            <td><a href="#" class="ins-qt-remove remove" title="Remove this Installer from your list" data-account-no="@c.AccountNumber">Remove installer</a></td>
        }
    </tr>
</table>

Now would it better to stay with this code or change it to do one loop with multiple stringbuilders:

StringBuilder company = new StringBuilder();
StringBuilder address = new StringBuilder();
StringBuilder contact = new StringBuilder();
StringBuilder remove = new StringBuilder();

foreach (Customer customer in Model.SelectedInstallers)
{
    company.AppendFormat("\n    <td class=\"company\">{0}</td>", customer.Name);
    address.AppendFormat("\n    <td class=\"address\">{0}<br />{1}<br />{2}<br />{3}</td>", customer.Street, customer.City, customer.County, customer.Postcode);
    contact.AppendFormat("\n    <td class=\"contact\">{0}<br />{1}</td>", customer.Telephone, customer.Website);
    remove.AppendFormat("\n    <td class=\"remove\"><a href=\"{0}\" class=\"remove-link\">Remove<span class=\"hide-element\"> {1} from the list</span></a></td>", removeUrl, customer.Name);
}

<table id="compamies-list" class="[email protected]">
    <tr class="first-row">
        <th scope="row">Company</th>
        @Html.Raw(company.ToString())
    </tr>
    <tr class="even">
        <th scope="row">Address</th>
        @Html.Raw(address.ToString())
    </tr>
    <tr class="odd">
        <th scope="row">Contact Details</th>
        @Html.Raw(contact.ToString())
    </tr>
    <tr class="even">
        <th scope="row">Remove</th>
        @Html.Raw(remove.ToString())
    </tr>
</table>

Or does it not matter if there is only going to be a maximum of ten installers

share|improve this question
    
First, it depends what your understanding of "better" is? Better in readability, performance, maintainability? As for performance, it really comes down to the list size. If there is only like 10 installers, there not much difference I guess –  derape Jul 23 '14 at 12:19
    
@derape, I'm guessing it's better in the sense of performance and scalability as it may change to more than 10 in future, although if it did I would probably change the table structure to go downwards instead of sideways so it wouldn't really matter in that case. –  Pete Jul 23 '14 at 12:45
    
This code is enumerating the Model a number of times. You can convert it to a list and reuse it. –  Sandeep Jul 23 '14 at 13:39

2 Answers 2

up vote 3 down vote accepted

Stay with the first solution:

  1. It is easier to read.

    Using such a template is the least-obfuscated method to generate the output HTML, as the structure of the template directly corresponds to the structure of the output.

    Another important aspect is that when embedding HTML into C# strings, you have to use many escapes. This makes it harder to read.

  2. I would expect the first template to have comparable or better performance.

    Instead of micro-optimizing, one should usually optimize for maintainability first, and let the compiler do its thing. Before optimizing for performance, do a benchmark to find real bottlenecks.

    I strongly suspect that the template engine is implemented in a non-moronic way, which means that internally a string builder is used. Your second solution on the other hand incurs additional overhead by maintaining multiple string builders which are then joined to an intermediate string which are then stuffed into the template – that intermediate string is unnecessary.

    But what about having fewer loops? The answer is that it doesn't matter from a theoretical perspective: Executing

    foreach (x in things)
        a(x);
    foreach (x in things)
        b(x);
    foreach (x in things)
        c(x);
    

    does the same amount of work as

    foreach (x in things)
    {
        a(x);
        b(x);
        c(x);
    }
    

    In practice there is some small overhead per loop, but this overhead is usually negligible when compared with the actual work done inside the loop. Do a benchmark to see if this overhead matters.

  3. It appears to be more secure.

    In your second solution, you directly interpolate arbitary strings into the HTML via AppendFormat. Unless you have very good input validation, this would allow for spectacular injection attacks. Better be safe than sorry, and escape everything. This is much easier if you use the templating engine rather than doing everything yourself – it's too easy to forget something.

There is another consideration from an UX standpoint: When reading tabular data, I expect each row to contain a record, and columns to contain different fields for that record. This makes such data easy to parse by humans and computers alike. Swapping the axes of your table also means that your template is ever simpler:

<table>
    <tr>
        <th>Company</th>
        <th>Address</th>
        <th>Contact Details</th>
        <th>Remove</th>
    </tr>

    @foreach (Customer c in Model.SelectedInstallers)
    {
        <tr>
            <td>
                @c.Name
            </td>

            <td>
                @c.Street   <br />
                @c.City     <br />
                @c.County   <br />
                @c.Postcode
            </td>

            <td>
                @c.Telephone<br />
                @c.Website
            </td>

            <td>
                <a      href="#"
                        title="Remove this Installer from your list"
                        data-account-no="@c.AccountNumber">
                    Remove installer
                </a>
            </td>
        </tr>
    }
</table>

While displaying data row after row scales very well to a large number of records, displaying items horizontally next to each other so that each item occupies a column makes it much easier to compare the data. However, this only scales well up to three items (in my experience), and horizontal scrolling is a big no-no.

Some notes regarding the HTML: The headings of columns or rows should use the <th> element. While you do this in the second template, the first template only uses plain <td>s. To style different rows differently, it is tedious to apply classes like first-row, even, or odd. These not only invite name collisions, but can also be done with much less effort via CSS pseudo-classes such as :nth-child:

/* tr.first-row */
tr:first-child { ... }
/* tr.even */
tr:nth-child(even) { ... }
/* tr.odd */
tr:nth-child(odd)  { ... }

The disadvantage of this solution is that it won't work on older MSIE versions. But in this case this wouldn't be a problem as the functionality of the table is not impaired – this is an example of progressive enhancement.

share|improve this answer
1  
I don't think I agree with your assertion about the loops. Yes, technically I suppose it does the same amount of work, but it does 3x the number of iterations. –  RubberDuck Jul 23 '14 at 12:33
    
I agree that readability and maintainability should ALWAYS be your first concern. Then, once you find that there are actually bottlenecks and issues with the performance of your code, THEN you go looking for optimizations. As the OP stated in the comments, the size of the collection is very small, and the entire structure would be reworked should it grow to an unwieldy size. That being the case, any "optimizations" are overkill. –  krillgar Jul 23 '14 at 12:49
    
Yeah I would have preferred swapping my axes but unfortunately the design department won't let me! as for your notes on html, you'll notice that my headings are indeed in th tags with a scope applied (as you should do with th tags - unlike yours) and the reason I have used the classes as I have is it needs to work and look like the design in ie8 (as this is what the directors of the company use) which nth-child doesn't. I see your point about readability and maintainability though. –  Pete Jul 23 '14 at 12:54
    
@ckuhn203 “this overhead is usually negligible when compared with the actual work done inside the loop” –  svick Jul 23 '14 at 17:05

It would be nice if you could separate the logic of creating the table and declaring what data should the table contain. I'm not a Razor expert, but it seems you can do this using helpers and templated delegates.

The helper to create the table can look like this (the naming could be improved):

@helper FormatData(IEnumerable items, params DataItem[] dataItems)
{
    <table class="key-left">
        @{
            bool even = false;
            foreach (var dataItem in dataItems)
            {
                <tr class="@(even ? "even" : null)">
                    <td>@dataItem.Heading</td>
                    @foreach (object obj in items)
                    {
                        <td>@dataItem.ValueSelector(obj)</td>
                    }
                </tr>
                even = !even;
            }
        }
    </table>
}

The DataItem is a simple class that contains the heading and a Razor templated delegate for the body:

public class DataItem
{
    public object Heading { get; private set; }
    public Func<dynamic, HelperResult> ValueSelector { get; private set; }

    // for simple value headings
    public DataItem(object heading, Func<dynamic, HelperResult> valueSelector)
    {
        Heading = heading;
        ValueSelector = valueSelector;
    }

    // for Razor headings
    public DataItem(
        Func<object, HelperResult> heading, Func<dynamic, HelperResult> valueSelector)
    {
        Heading = heading(null);
        ValueSelector = valueSelector;
    }
}

Now you can use it like this:

@FormatData(Model.SelectedInstallers,
    new DataItem("Company", @<text>@item.Name</text>),
    new DataItem("Address",
     @<text>@item.Street<br />
            @item.City<br />
            @item.County<br />
            @item.Postcode</text>),
    new DataItem(@<text>Contact Details</text>, // heading can use Razor syntax too
     @<text>@item.Telephone<br />
            @item.Website</text>),
    new DataItem("Remove",
     @<a href="#" class="ins-qt-remove remove" title="Remove this Installer from your list" data-account-no="@item.AccountNumber">Remove installer</a>))

The code is now much more DRY: if you wanted to change how odd rows behave, use <th> for headings or switch to the more common vertical table layout, you will do it only in one place and it will be much less error-prone.

The downside of this is that the value selectors are not strongly typed (e.g. the compiler won't complain if you write @<text>@item.CompanyName</text> instead of @<text>@item.Name</text>), because I didn't figure out a way to do that while still keeping the calling code relatively simple.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.