97

I heard that having @foreach inside of a view is a no-no. Meaning, the view should not have any logic in it. What is the best practice on where the logic for the @foreach should be at?

    @foreach.. 
tereško
  • 58,060
  • 25
  • 98
  • 150
Nate Pet
  • 44,246
  • 124
  • 269
  • 414

5 Answers5

193

What is the best practice on where the logic for the @foreach should be at?

Nowhere, just get rid of it. You could use editor or display templates.

So for example:

@foreach (var item in Model.Foos)
{
    <div>@item.Bar</div>
}

could perfectly fine be replaced by a display template:

@Html.DisplayFor(x => x.Foos)

and then you will define the corresponding display template (if you don't like the default one). So you would define a reusable template ~/Views/Shared/DisplayTemplates/Foo.cshtml which will automatically be rendered by the framework for each element of the Foos collection (IEnumerable<Foo> Foos { get; set; }):

@model Foo
<div>@Model.Bar</div>

Obviously exactly the same conventions apply for editor templates which should be used in case you want to show some input fields allowing you to edit the view model in contrast to just displaying it as readonly.

Darin Dimitrov
  • 1,023,142
  • 271
  • 3,287
  • 2,928
  • I agree with @DarinDimitrov that the second example looks and reads better. Although i dont see a problem with a foreach loop inside the display either! – Nicholas King Jun 29 '12 at 12:20
  • @NicholasKing, there's a problem with generating proper names for the input fields of editor templates. For example just look at the [answer of Noobsyke](http://stackoverflow.com/a/11261698/29407) below. Replace his DisplayFor with an EditorFor and the model binder won't work because he hasn't respected the conventions. – Darin Dimitrov Jun 29 '12 at 12:21
  • 4
    @DarinDimitrov thats true if its a strict MVC model you are coding against however if a circumstance arises where a collection needs to be looped through that is not in the model, i dont see a problem with using foreach – Nicholas King Jun 29 '12 at 12:24
  • 6
    @NicholasKing, such circumstance should never arise. A view shouldn't touch anything else than what's present in the view model passed by the controller action. If it is not in the view model then you should put it in there if the view needs it. That's exactly what view models are meant for. – Darin Dimitrov Jun 29 '12 at 12:25
  • @DarinDimitrov are you saying my example is bad practice? – Mihai Labo Jul 03 '12 at 13:17
  • 1
    @MihaiLabo, yes, that's what I am saying. – Darin Dimitrov Jul 03 '12 at 14:36
  • 2
    What if the type of the element in the collection is not enough to determine the display? For example, my model has a collection of strings, but sometimes I want to have one string per line and in another case I want them separated by commas? – Marc Stober Jan 31 '14 at 21:00
  • 7
    So, what exactly is the the problem with `foreach`? At the very least a display template (while a perfectly acceptable approach, regardless) requires a new view to be rendered, which is not free. The majority of the time it won't affect your site load time noticeably, but done enough, it could cause a performance hit. A `foreach` around a bit of HTML is and always will be virtually instantaneous. Like I said, it's not a huge deal either way, though, but if anything there's an argument *for* using `foreach`. – Chris Pratt Jul 14 '14 at 16:49
  • Darin Thank You SO Much! This is so much more maintainable and elegant than using a bunch of `for` loops. Great thanks for sharing your understanding of editor templates with us! P.s. this solution works well with your other solution http://stackoverflow.com/questions/11234771/create-a-form-from-a-recursive-model – user1477388 Dec 08 '14 at 15:53
  • Hi Darin, you suggest using `@Html.DisplayFor(x => x.Foos)` but what if my model is the collection? Should it be `@Html.DisplayFor(x => x)` or is there a more appropriate way to do this? Can you provide an example for this use case in your answer please? – Ben Aug 20 '15 at 08:20
  • @Darin What if I have the same nested list viewmodel in several places, the display template template will be different for each of these but they will all have the same @Html.DisplayFor(x => x.Foos) which will reference to the same display template? Do i need a UIHint for each of these list viewmodels? – Mindless Jun 02 '16 at 06:29
  • Helped me because I needed to know how I use the @foreach in a view :) – maracuja-juice Sep 05 '17 at 10:48
  • 1
    The problem I have with this is that it hurts maintainability, in certain cases. When I first began developing, I found several DisplayFors with DisplayFors, and it took me forever to find the actual views associated with these, because F12 on DisplayFor didn't take you to the associated view. My personal practice is to type the string for the associated view, even though it is redundant. This provides quick F12 interaction with Visual Studio, and helps anyone trying to look at my code. – Michael Sefranek Jan 29 '18 at 18:14
115

When people say don't put logic in views, they're usually referring to business logic, not rendering logic. In my humble opinion, I think using @foreach in views is perfectly fine.

JonoW
  • 14,029
  • 3
  • 33
  • 31
  • 26
    Agreed. Reminds me of the old semantic HTML debates, that eventually led people to trying to use divs and CSS to create a "table" for actual tabular data, because they were so anti-table. – Chris Pratt Jul 14 '14 at 16:52
  • 1
    I agree. Humans are bad at mis-direction. Do I really need a new folder, with a new view, just to display things in a list in my viewmodel? – Don Cheadle Sep 29 '16 at 15:48
  • 3
    Wouldn't using div/css to create tabular data view be necessary for creating a responsive view? – frostshoxx Oct 24 '17 at 17:19
16

I'm using @foreach when I send an entity that contains a list of entities ( for example to display 2 grids in 1 view )

For example if I'm sending as model the entity Foo that contains Foo1(List<Foo1>) and Foo2(List<Foo2>)

I can refer to the first List with:

@foreach (var item in Model.Foo.Foo1)
{
    @Html.DisplayFor(modelItem=> item.fooName)
}
Xm7X
  • 861
  • 1
  • 11
  • 23
Mihai Labo
  • 1,082
  • 2
  • 16
  • 40
11

a reply to @DarinDimitrov for a case where i have used foreach in a razor view.

<li><label for="category">Category</label>
        <select id="category">
            <option value="0">All</option>
            @foreach(Category c in Model.Categories)
            {
                <option title="@c.Description" value="@c.CategoryID">@c.Name</option>
            }
        </select>
</li>
Nicholas King
  • 938
  • 7
  • 22
  • 6
    WOW man, you would write something like this in a view? Why not write a reusable custom helper à la `Html.DropDownListFor` that will simply take into account the title? It's trivial and doesn't turn your views into spaghetti code: http://stackoverflow.com/a/7938038/29407 – Darin Dimitrov Jun 29 '12 at 12:33
  • 8
    @DarinDimitrov yes we work in a very agile environment, which means that scenarios like this sometimes prevent us using things like DropDownFor as we dont always have a clearly defined requirement. I believe in this case the drop down initially didnt need "all" then it did but only in one DropDown on the view. As this page uses ajax to update its not a strict MVC pattern and you cant upload a product into all categories per the requirement. Not ideal but sometimes unavoidable. – Nicholas King Jun 29 '12 at 13:07
  • 1
    Perhaps a better example would be using this to render `optgroup` elements in the select list, since there's no support for that in the HtmlHelpers. If you just need to add an additional item to the select list, there's better ways to achieve that and then still use the helper. – Chris Pratt Jul 14 '14 at 16:57
  • That would not be my definition of Agile - I have to agree with @DarinDimitrov – Luis Filipe Feb 12 '16 at 02:25
5

The answer will not work when using the overload to indicate the template @Html.DisplayFor(x => x.Foos, "YourTemplateName) .

Seems to be designed that way, see this case. Also the exception the framework gives (about the type not been as expected) is quite misleading and fooled me on the first try (thanks @CodeCaster)

In this case you have to use @foreach

@foreach (var item in Model.Foos)
{
    @Html.DisplayFor(x => item, "FooTemplate")
}
Community
  • 1
  • 1
Tiberiu Craciun
  • 3,231
  • 1
  • 13
  • 12
  • 1
    That answer is written by someone who works on MVC, so I guess they know what they are saying. MVC will iterate the `IEnumerable` and call the template for type `T` for each element. – CodeCaster Sep 05 '15 at 11:11
  • Regarding your edit: it's either your code that's wrong or a bug in that specific MVC version (in 5.2.2 it works for me). It is supposed to work as described in the accepted answer. Instead of saying it is wrong, open your own question about the issue if you want. – CodeCaster Sep 05 '15 at 11:47
  • @CodeCaster I believe my response adds some heads up for that specific case (it wasted some of my time to figure out what went wrong). Will you please add some explanation for keeping the downvote? (thanks for your time btw, just want to get to the bottom of things) – Tiberiu Craciun Sep 06 '15 at 18:05
  • I don't think it's a good answer as it does not verify the bug, this helps rumors in the world like "A DisplayTemplate can't iterate" - it is supposed to, so it should work. If it really doesn't, file a bug and rather open a separate question where you reproduce the issue and mention that this specific version, as opposed to under a Q&A that handle what normally should happen. – CodeCaster Sep 06 '15 at 18:45
  • @CodeCaster I had another edit meanwhile after I understood my specific case (I changed the entire post). In my last edit I indicated the exact condition when the accepted answer doesn't apply with backup link to a relevant question answered by the same guy proving that's by design and not a bug. – Tiberiu Craciun Sep 06 '15 at 19:21