2
\$\begingroup\$

I've created this script and I want to use it for website navigation in my project. As my knowledge of jQuery is quite poor, I'm not sure that code is correct and enough optimized. Could you help me to find out if there are any ways to optimize the code, and indicate mistakes I've made?

$(function () { //scroll to the section, section's id is equal to li href attribute
    $("li a").click(function () {
        var liC = $(this).attr("href");
        var liD = $("#" + liC).offset().top;
        $("html, body").stop().animate({
            scrollTop: liD
        }, 'slow');
        return false;
    });

    $(window).scroll(function () { //on scrolling defines the closets section gets it's id finds li item the href of which is equal to section's id and adds padding to it

        $('.dober').each(function () {
            var hRt = $(this).attr('id');
            if ($(this).offset().top <= $(window).scrollTop()) {

                $('li a').css({ padding: 0 });                  
                $('a[href=' + hRt + ']').css({
                    padding: 10
                });

            } else {

                 $('a[href=' + hRt + ']').css({
                    padding: 0
                });
            }
        });
    });
});
\$\endgroup\$
  • 1
    \$\begingroup\$ I've read your code a couple of times and must say that I don't really understand what it is doing. It would probably be more readable if you used more verbose variable names in stead of names like liC, liD and hRt. \$\endgroup\$ – Michael Zedeler Jun 9 '13 at 14:02
  • \$\begingroup\$ Here is the fiddle that shows how the script works. jsfiddle.net/Cone/fAz7p/1 \$\endgroup\$ – Cone Enoc Jun 9 '13 at 19:18
1
\$\begingroup\$

- Cache your selectors: As a rule of thumb, if you use a selection more than once, you should cache it. What happens when you use $("someElem") is jQuery has to jump into the DOM and look through all elements that would match that selection. So you should really do a search only once, and save your results for future use. This way you can look and play with them whenever you want. Ex.:

$("a[href=" + hRt + "]").css({
    padding: 10
});

//Should be like this:
var whichLink = $("a[href=" + hRt + "]"); //Saved my search to a variable, which I use later.
whichLink.css({
    padding: 10
});

//...

whichLink.css({
    padding: 0
});

- Save function calls: If you read the jQuery source code you'll see that shortcut methods like .click(), .scroll(), and etc. all reference the .on() method. All they do is basically call the .on() method with some parameters. Well why not just go straight to the meat and potatoes? Check this out:

$("#foo").click(function() {
    //Do your awesomeness
});

//That does the same thing as this:
$("foo").on("click", function() {
    //Do your awesomeness even awesomener.
});

//Or try this:

$(window).on("scroll", function () {
    //Wicked!
});

That just makes the syntax so easy to rememeber as well. If you're going to use jQuery on a regular basis, I recommend that you take some time and read through the source code of the methods you're using. To find them quickly just use CTRL+F and type methodName:. That should jump you straight to what you want to know. This way you can understand what and how you are doing things, and even find better ways to do them on your own. Also if you see something you think should be done better or differently, you know how it works and you can contribute to jQuery.

In your click functions you also might want to prevent the default browser action on a link, which is to direct the page to that link. Since you just want to perform something on your page and don't actually want the browser to leave the page you should prevent that action. You can do it by passing in e for event and running e.preventDefault();. I see you've used return false; which does that, but if you want to read more on the difference between the two I would suggest [this article by Chris Coyier][1].

-Playing with visible elements: You're if statements from what I can tell are trying to detect scroll position and etc. I highly recommend you check out [this tiny plugin][2]. It basically detects if an element is visible or not at any given time. This may or may not help you out but I just thought I'd throw that out there.

-Performance Wise: Focus on the .scroll() event for now. The way you have it now, the code runs hundreds of times since it is called each time the window scrolls, even if the user isn't done scrolling. That's an increadible ammount of times your code will run for no reason. Not only that but then you run code on each element with the class of .dober - more code that gets run. Now we don't want to do that since it can really slow down the browser, if not crash it all together. What you'll want to do is wait for when the user stops or is done scrolling, then run the code.

var $window = $(window); //Remember, if you use a selection more than once, you should cache it

$window.on(function() {
    //Here we check if there has been a call already
    //If the check is true that means the code was already called
    //So we cancel any previous calls, this way the code is only called once
    if(this.scrollEnd) clearTimeout(this.scrollEnd);

    //If you've made it here, the code hasn't been called, or the call has been canceled.
    this.scrollEnd = setTimeout(function() {
        $(this).trigger('scrollEnd'); //Call the actual code with a 500ms delay
    }, 500);
});

$window.on('scrollEnd', function() {
//This is a custom event that is triggered after the timeout
//This will only run on the last time the scroll method was called
    $('.dober').each(function () {
        var hRt = $(this).attr('id');
        var paddingAmt = 0; //Here I just slightly optimized your code

        if ($(this).offset().top <= $window.scrollTop()) {
            paddingAmt = 10;
            $('li a').css({ padding: 0 });
        }

        $('a[href=' + hRt + ']').css({
            padding: paddingAmt
        }, function(){ //Callback to set padding back to zero
            paddingAmt = 0;
        });
    });
});


[1]: http://css-tricks.com/return-false-and-prevent-default/
[2]: https://github.com/teamdf/jquery-visible/
\$\endgroup\$
  • \$\begingroup\$ Dear Jonny, thx a lot for your comments and clear explanations, that was really helpful for me! \$\endgroup\$ – Cone Enoc Jun 10 '13 at 17:40

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.