1
\$\begingroup\$

I have the following code, though I'm not sure it is efficient as it could be.

 $(window).scroll(function() {
    var scrollT = $(document).scrollTop();
    if (scrollT >= 180) {
        $("#primary-nav-wrapper").addClass("scroll");
        $("#primary-nav-wrapper li.front").addClass("active");


        $("#primary-nav-wrapper .search-wrapper").removeClass("active");
        $("#primary-nav-wrapper .search-field").blur();
    }
    else {
        $("#primary-nav-wrapper").removeClass("scroll");
        $("#primary-nav-wrapper .search-wrapper, #primary-nav-wrapper li.front").removeClass("active");

        $("#primary-nav-wrapper .search-field").blur();
    }
    if (scrollT >= 400) {$("a#to-top-link").addClass("active");}
    else {$("a#to-top-link").removeClass("active");}
});

Basically, what I do here is checking two if-clauses every time I scroll, but is it more resource-friendly to only check every few milliseconds? If so, how is this done? Or is it a better idea to re-write the if-else structure, e.g. if.. else if... else if... else?

\$\endgroup\$
  • \$\begingroup\$ What does setting the active class accomplish? Could you make a live Stack Snippet example including some HTML and CSS? \$\endgroup\$ – 200_success Jan 27 '15 at 20:36
  • \$\begingroup\$ @200_success The classes trigger CSS3 animations, which I prefer over JS enabled ones. I don't think a live example would be helpful because the code works perfectly, but I was simply wondering if it could be optimised a bit. \$\endgroup\$ – Bram Vanroy Jan 28 '15 at 10:30
1
\$\begingroup\$

I don't know if you mean general performance, but one thing I've heard helps a lot is to put all of your different selectors in variables outside of the function as it is apparently pretty expensive to make the selector calls again and again. I mean something like this (and I do mean "something like this" because I haven't tested even the basics, but the general idea is described here: http://www.artzstudio.com/2009/04/jquery-performance-rules/#cache-jquery-objects)

var scroll = {
     primary  : $("#primary-nav-wrapper"),
     front    : $("#primary-nav-wrapper li.front"),
     search_w : $("#primary-nav-wrapper .search-wrapper"),
     search_f : $("#primary-nav-wrapper .search-field"),
     to_top   : $("a#to-top-link")
}
$(window).scroll(function() {
    var scrollT = $(document).scrollTop();
    if (scrollT >= 180) {
        scroll.primary.addClass("scroll");
        scroll.front.addClass("active");


        scroll.search_w.removeClass("active");
        scroll.search_f.blur();
    }
    else {
        scroll.primary.removeClass("scroll");
        scroll.front.removeClass("active");

        scroll.search_f.blur();
    }
    if (scrollT >= 400) {scroll.to_top.addClass("active");}
    else {scroll.to_top.removeClass("active");}
});
\$\endgroup\$
  • \$\begingroup\$ Would it possibly be an even further optimisation when saying for instance front: primary.find("li.front")? \$\endgroup\$ – Bram Vanroy Jan 28 '15 at 10:29
  • \$\begingroup\$ Sure. But a very minor one as you're only doing it once. Mostly in your case, you want to keep the dom traversal out of the "scroll" part because that is firing a lot (on every user scroll). For the set up, which runs only once per page load, I'd go with whatever is clearer to you. \$\endgroup\$ – dgig Jan 28 '15 at 15:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.