current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite
1

I am using this code to get images to load only if they are visible. However, it seems to be slow with thousands of images, even if they are not rendered.

function getViewportHeight() {
    if(window.innerHeight) {
        return window.innerHeight;
    }
    else if(document.body && document.body.offsetHeight) {
        return document.body.offsetHeight;
    }
    else {
        return 0;
    }
}
function inView(elem, nearThreshold) {
    var viewportHeight = getViewportHeight();
    var scrollTop = (document.documentElement.scrollTop ? document.documentElement.scrollTop : document.body.scrollTop);
    var elemTop = elem.offset().top;
    var elemHeight = elem.height();
    nearThreshold = nearThreshold || 0;
    if((scrollTop + viewportHeight + nearThreshold) > (elemTop + elemHeight)) {
        return true;
    }
    return false;
}

function loadVisibleImages() {
    jQuery('img[data-src]').each(function() {
        if(jQuery(this).is(':visible')) {
            if(inView(jQuery(this), 100)) {
                this.src = jQuery(this).attr('data-src');
            }
        }
    });
}

jQuery(window).scroll(function() {
    loadVisibleImages();
});

I am using this code to render images:

<img src="" data-src="http://placehold.it/400x400" alt="" width="400" height="400">

Everything works, but how can I optimize it?

share|improve this question

3 Answers 3

up vote 1 down vote accepted

Here are a few callouts/suggestions:

You should throttle your code on the scroll event. That way the event doesn't fire thousands of times. You can read up this article for more information. A basic example would be: 

//which makes the `scroll` event fire every 250ms.
$(window).scroll( $.throttle( 250, loadVisibleImages) );

You probably don't need to get the viewport size every time through the loop. This should be called once at the start and then only on the resize event. Otherwise, it never changes so there is no need to re-calculate it.

//which makes the `resize` event fire every 250ms.
$(window).resize( $.throttle( 250, getViewportHeight) );

You can add a class to all the images that have already been processed and exclude them from your selector. This way you are only dealing with new images. For example:

function loadVisibleImages() {
  jQuery('img[data-src]').not('.processed').each(function() {
    if(jQuery(this).is(':visible')) {
        if(inView(jQuery(this), 100)) {
            this.src = jQuery(this).attr('data-src').addClass('processed');
        }
    }
  });
}

If you haven't already you should wrap your code in an IIFE so you can create a private scope. Just take the code below and put it around your existing code. This would also allow you to use $ for jQuery without worry.

(function( $ ) {
  //your code here
}) ( jQuery );

You could optimize the loop in your loadVisibleImages functions. See your other question.

Hope that helps!

share|improve this answer
    
Very valid points with the resize and the throttle functions. Let me test the code and I'll get back to you. –  Ciprian yesterday
    
I had to rewrite the function, as jQuery :visible is quite slow, so I decided to implement the function for each AJAX call. But the answer is correct and it speeds up the function a lot. –  Ciprian yesterday
up vote 2 down vote

I am not a fan of nesting statements more than you have to, I would suggest that you un-nest this piece of code a little bit to make it a little bit cleaner(in my opinion)

function loadVisibleImages() {
    jQuery('img[data-src]').each(function() {
        if(jQuery(this).is(':visible')) {
            if(inView(jQuery(this), 100)) {
                this.src = jQuery(this).attr('data-src');
            }
        }
    });
}

like this

function loadVisibleImages() {
    jQuery('img[data-src]').each(function() {
        if(jQuery(this).is(':visible') && inView(jQuery(this), 100)) {
            this.src = jQuery(this).attr('data-src');
        }
    });
}

It is essentially the same thing that you have, except for that it is not nested as deeply.

share|improve this answer
up vote 1 down vote

In method inView you recount variables which are the same for every scroll position: 

viewportHeight, scrollTop,nearThreshold and their sum

You can count them once in the scroll event handler and then pass to inView only their sum.

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.