Multiple loops versus multiple stringbuilders

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

    
        Company
        @foreach (Customer c in Model.SelectedInstallers)
        {
            @c.Name
        }
    
    
        Address
        @foreach (Customer c in Model.SelectedInstallers)
        {
            
                @c.Street
                @c.City
                @c.County
                @c.Postcode
            
        }
    
    
        Contact Details
        @foreach (Customer c in Model.SelectedInstallers)
        {
            
                @c.Telephone
                @c.Website
            
        }
    
    
        Remove
        @foreach (Customer c in Model.SelectedInstallers)
        {
            Remove installer
        }
    

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    {0}", customer.Name);
    address.AppendFormat("\n    {0}{1}{2}{3}", customer.Street, customer.City, customer.County, customer.Postcode);
    contact.AppendFormat("\n    {0}{1}", customer.Telephone, customer.Website);
    remove.AppendFormat("\n    Remove {1} from the list", removeUrl, customer.Name);
}

    
        Company
        @Html.Raw(company.ToString())
    
    
        Address
        @Html.Raw(address.ToString())
    
    
        Contact Details
        @Html.Raw(contact.ToString())
    
    
        Remove
        @Html.Raw(remove.ToString())
    

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

Stay with the first solution:

-
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.

-
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.

-
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:

    
        Company
        Address
        Contact Details
        Remove
    

    @foreach (Customer c in Model.SelectedInstallers)
    {
        
            
                @c.Name
            

            
                @c.Street   
                @c.City     
                @c.County   
                @c.Postcode
            

            
                @c.Telephone
                @c.Website
            

            
                
                    Remove installer
                
            
        
    }

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 ` element. While you do this in the second template, the first template only uses plain 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.

StackExchange Code Review Q#57773, answer score: 3