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 wrote an infinite scrolling scrolling plugin for an app I'm developing. When requesting the second 'page' from the server, I loop through each image, and five it a very basic onload function.

// I will have the result in a variable called data
// Fetch each image from result
$images = $( data.content ).find('img');
// Cache length for use in for loop
imagesLength = $images.length;

for ( var i = 0; i < imagesLength; i++ ) {
    // Create new Image object
    img = new Image();
    // Match src of image in the result
    img.src = ( $images[i].src );
    // Add onload listener
    img.onload = function () {
        // Increment iterator, and check if we've loaded all the images
        if( ++imageLoadingIterator == images.length) 
            _continue(); // Run _continue() if this is the case
    }
}

I feel like this isn't the best way to do this. Is there a way I can improve this function to run faster? For example: maybe creating a new Image object for each img tag is too much overhead?

share|improve this question
    
Why do you think it can be sped up? Because as far as I can tell, the js can't be made much faster. If it seems slow, it's probably the actual image fetching that's taking time –  Flambino Mar 8 '13 at 0:26

1 Answer 1

up vote 0 down vote accepted

Not really about performance, but this is more about clarity of the code.

  • Attach onload handlers first, for readability. Appending the src starts the loading process. If you saw code that loaded something, expect to do something, but haven't found the handler to do it makes it a bit confusing.

  • Declare one handler function in a persistent scope and have the onload refer to it, rather than create one handler for each onload, or creating the function everytime the whole routine executes.

  • Instead of find(), use jQuery function's second parameter, context. With this, jQuery finds the elements with the given selector under the jQuery object that is on the second parameter.

  • If the images are children of data.content, use children() instead of find() to avoid deep DOM traversal. Also, the lower you are in the DOM, the less deep find() has to dive and find your elements.

  • Use strict comparison as much as possible.

  • Don't forget to declare the variables and use var. You'd be declaring globals if you didn't.

And so:

var $images = $('img', data.content),
  imagesLength = $images.length,
  imageLoadingIterator = 0,
  img, i;

function onImageLoad() {
  if (++imageLoadingIterator === images.length) _continue();
}

for (i = 0; i < imagesLength; i++) {
  img = new Image();
  img.onload = onImageLoad;
  img.src = $images[i].src;
}

However, these advices might have a very negligible gain in performance, and might be unnecessary since the compiler might be optimizing them in some way.

share|improve this answer
    
Thanks for the tips! I defined the variables as you recommended already, but I forgot to paste it into the question. It was behind some other (irrelevant) code so I missed it –  Martin Mar 8 '13 at 17:21

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.