Update append-outside-loop.md #445

Closed
wants to merge 2 commits into
from

Projects

None yet

6 participants

@AurelioDeRosa
jQuery Foundation member

Updated the last example adopting a for loop and length cache. We should recommend how to achieve the best performance.

@AurelioDeRosa AurelioDeRosa Update append-outside-loop.md
Updated the last example adopting a for loop and length cache. We should recommend how to achieve the best performance.
34d520b
@mgol
jQuery Foundation member

Caching the array length isn't really important in current browsers.

@AurelioDeRosa
jQuery Foundation member

@mzgol Basically the PR was made to change the $.each with the for. Then I included the caching because it's currently included in the doc.

@kswedberg kswedberg and 1 other commented on an outdated diff Dec 4, 2013
page/performance/append-outside-loop.md
@@ -37,16 +37,17 @@ $.each( myArray, function( i, item ) {
$( "#ballers" )[ 0 ].appendChild( frag );
```
-Another simple technique is to build up a string during each iteration of the loop. After the loop, just set the HTML of the DOM element to that string.
+Another simple technique is to build up a string during each iteration of the loop. To have better performances, the iteration should use a straight `for` loop with [cached length](http://learn.jquery.com/performance/cache-loop-length/). After the loop, just set the HTML of the DOM element to that string.
@kswedberg
kswedberg Dec 4, 2013

"performances" should be "performance" (singular). A couple other tweaks would be helpful, too:

For better performance, the iteration could use ...

I like "could" here, because $.each() is not always a performance problem. If you're only iterating over handful of items (and not doing so thousands of times), it's not going to be an issue.

@AurelioDeRosa
AurelioDeRosa Dec 4, 2013

@kswedberg I agree on the use of could instead of should. Updated using your suggestions.

@scottgonzalez
jQuery Foundation member

I'd much rather see the example stay as-is and just add a sentence about switching to for and linking to the loop caching article at the end. The overhead of $.each() is pretty low, especially compared to DOM modifications.

The reader should be focusing on the purpose of the article, which is reducing the number of times the DOM is touched. We shouldn't distract them with for loops and length caching. Especially since it changes scoping.

@AurelioDeRosa
jQuery Foundation member

@scottgonzalez A change in a loop statement doesn't seem much distracting to me (1 line) but add value to the information. Anyway, the decision of merge it or not, it's up to you.

@scottgonzalez
jQuery Foundation member

It's more than one line. It's a complete change in the style. You've lost item and now need to use array references, and variables declared inside the loop don't have their own scope anymore.

@tjvantoll
jQuery Foundation member

I agree with @scottgonzalez. The point of this article is to avoid DOM interactions and this moves the focus away from that.

@ajpiano
jQuery Foundation member

I'm with @scottgonzalez and @tjvantoll on this particular issue -- the point here is to cut down on reflows, not improve performance by changing the loop style -- pehaps we should mention that specifically

@AurelioDeRosa AurelioDeRosa deleted the AurelioDeRosa:patch-2 branch Jul 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment