3
\$\begingroup\$

What is the best way to tidy up JavaScript code I have written? It seems very long winded, any suggestions to shorten it?

Note: I'm running it on Wordpress which runs jQuery in nonconflict mode, hence the insane amounts of jQuery.().

jQuery("document").ready(function (jQuery) {

    if (jQuery(window).width() < 960) {
        jQuery(".main").animate({
            "top": "22%"
        });
        jQuery('#tblcontents').toggle(function () {
            jQuery(".main").fadeOut(1000);
        }, function () {
            jQuery(".main").fadeIn(1000);
            jQuery(".main").animate({
                "top": "22%"
            });

        });

    } else {}

    jQuery('.menu-button').click(function () {
        jQuery("#sb-container div").css("transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-webkit-transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-ms-transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-moz-transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-o-transform", "rotate(0deg)");
        jQuery(".main").animate({
            "right": "1%"
        }, "slow");
    });

    jQuery(function () {
        jQuery('.toggle_div').toggle(function () {
            jQuery(".main").animate({
                "right": "50%"
            }, "slow");
        }, function () {
            jQuery(".main").animate({
                "right": "1%"
            }, "slow");
        });
    });

    jQuery(".pulse").effect("pulsate", {
        times: 100
    }, 2000).click(function () {
        jQuery(".pulse").effect().stop(true, true);
        jQuery(".pulse").animate({
            "opacity": "1"
        }, "fast");
    });
    jQuery('.thumb').click(function () {
        jQuery("#sb-container div").css("transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-webkit-transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-ms-transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-moz-transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-o-transform", "rotate(0deg)");
        jQuery("#sb-container").animate({
            "left": "0px"
        }, "slow");
        jQuery(".main").animate({
            "right": "1%"
        }, "slow");
    });

    jQuery(window).scroll(function () {
        if (jQuery(this).scrollTop() > 20) {
            jQuery("#sb-container div").css("transform", "rotate(0deg)");
            jQuery("#sb-container div").css("-webkit-transform", "rotate(0deg)");
            jQuery("#sb-container div").css("-ms-transform", "rotate(0deg)");
            jQuery("#sb-container div").css("-moz-transform", "rotate(0deg)");
            jQuery("#sb-container div").css("-o-transform", "rotate(0deg)");
            jQuery(".main").animate({
                "right": "1%"
            }, "slow");

        } else {
            jQuery("#sb-container").draggable()
        }
    });

});

jQuery(function () {
    jQuery("#sb-container").draggable()
});

How do I go about reducing the amount of code shown here? What are the best practices and what am I missing? Also, do I need the else statements querying the window width since they don't have any functions in them for the most part?

\$\endgroup\$
8
\$\begingroup\$

I'm going to go through a few main points here about your code and at the bottom I'll have a "what I would change" section with inline comments. Do not be overwhelmed by the size of this book, I prefer to be on the side of too much information than too little.

- Cache your selectors: Probably the most important thing you can do for your code now. 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 queries the DOM to try and find elements that match. So imagine that every time you do that it runs a search. Would it be better if you could save the search results? This way you can look and play with them whenever you want. Ex.:

$("#sb-container div").css("transform", "rotate(0deg)"  );
$("#sb-container div").css("-webkit-transform", "rotate(0deg)"  );

//Should be like this:
var sbContainer = $("#sb-container div"); //Saved my search to a variable, which I use later.
sbContainer.css("transform", "rotate(0deg)"  );
sbContainer.css("-webkit-transform", "rotate(0deg)"  );

- You can use $ in Wordpress: If you have your code in the footer (which you should be doing) what you'll want to do is wrap it in a IIFE. "What the hell is that?" you may ask. A basic syntax looks like this:

(function () {
    //Code goes in here.
})();

You'll see several variations of this all over the internet as you read to find out more. The one I recommend you use in this case is like this:

;(function ($, window, document) {
    //Code goes here and the $ belongs to jQuery
})(jQuery, window, document);

So lets break this down. The ; in the front is a safety net against unclosed scripts, which can be common if you use plugins in Wordpress, or if you concat and minify your files it will protect you from your function becoming an argument.

Then we pass in $, window, and document. We pass in $ and at the bottom assign it to jQuery. So now in this function, no matter what value the $ carried outside, in here it's jQuery. Then we pass in window and document. These are optional in your case, but a good idea since you do use references to window in your code. It saves window as a local variable, and also will be good when you minify your code as the window reference can be changed to something like a or foo automatically.

Now keep in mind there might be times where you have to put your script in the header. Modernizr is an example of this. Then you'll probably end up using the document.ready. Don't sweat because all you have to do is this:

jQuery(document).ready(function( $ ) {
    //Code goes here and $ will belong to jQuery.
});

I wouldn't rely on $.noConflict

- Chaining: As pointed out in your question, you should be using something like that. jQuery is particularly good at this and you should make the most of it. Ex.:

$("#someElem").css("background-color", "#fff").animate({"right":"1%"}, "slow").draggable();

I apply all this stuff to this selector, one after the other. If I want to continue chaining but change my selectors, I can use stuff like .find() and continue with my methods.

- 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 variances. 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.
});

I highly recommend, if you're going to use jQuery on a regular basis, 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 method:. 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.

-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. 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.

-Things I'd write differently: Here's an example of all of this summed up into your actual code. I've included comments so you can relate to the reading part.

;(function ($, window) { //Here's our IIFE giving us full $ power
    //Right here at the top we declare our selection variables to be used throught the code.
    //Don't forget to replace your selectors with the appropriate variable name.
    var main = $(".main"),
        sbContainer = $("#sb-container"),
        sbContainerDiv = sbContainer.find("div");

    function animate() { //Write your code only once, and call it when you need it.
        sbContainerDiv.css({ //You should apply a class instead of doing all of this in here like this.
            "transform": "rotate(0deg)",
            "-webkit-transform": "rotate(0deg)",
            "-ms-transform": "rotate(0deg)",
            "-moz-transform": "rotate(0deg)",
            "-o-transform": "rotate(0deg)"
        });

        main.animate({"right":"1%"}, "slow");
    }

    if ( window.width() < 960) {
        main.animate({"top":"22%"});

        //This use of .toggle() was deprecated in jQuery 1.8 and removed in 1.9
        //Consider doing this differently
        //If you provide me with more details on this I can help you come up with a different solution
        $('#tblcontents').toggle(function() {
            main.fadeOut(1000);
        }, function() { 
            main.fadeIn(1000);
            main.animate({"top":"22%"});
        });
    } else {
        $('.menu-button').on('click', function() {
            animate(); //Here we call our previously set animate function
        });
    }

    //Refer to the previous toggle comment.
    $('.toggle_div').toggle(function() {
        main.animate({"right":"50%"}, "slow");
    }, function(){ 
        main.animate({"right":"1%"}, "slow");
    });

    $(".pulse").effect("pulsate", { times:100 }, 2000).on('click', function() {
        $(this).effect().stop(true, true); //In a callback function, 'this' refers to the element in question.
        $(this).animate({"opacity":"1"}, "fast");
    });

    $('.thumb').on('click', function() {
        animate();
    });

    window.on('scroll', function() {
        if ($(this).scrollTop() > 20) {
            animate();
        } else{
            sbContainer.draggable();
        }
    });
    //You only need to make this draggable once, either here or in your else statement.
    //Remove one depending on what you need.
    sbContainer.draggable()

})(jQuery, window); //Here we set our awesome stuff in the function

UPDATE

I've looked at your .js file and there's quite a bit I would change. But focus on the resize event for now. The way you have it now, the code runs hundreds of times since it is called each time the window size changes, even if the user isn't done resizing.

Now we don't want to do that since it can really slow down the browser, if not crash it all together. So what you want to do is only run when the user stops or is done resizing, then run the code. Here's an example of how I'd probably do it:

var $window = $(window); //Remember, if you use a selection more than once, you should cache it.
$window.resize(function() {
    //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.resizeTO) clearTimeout(this.resizeTO);

    this.resizeTO = setTimeout(function() {
        $(this).trigger('resizeEnd'); //Call the actual code with a 500ms delay
    }, 500);
});

$window.on('resizeEnd', function() {
//This is a custom event that is triggered in the timeout
//This will only run on the last time the resize method was called
    var winWidth = $window.width();

    if( winWidth < 960 ) {
        toggletwo();
        animate();
    } else {
        toggleone();
    }
});
\$\endgroup\$
  • \$\begingroup\$ you are my new best friend thank you for the detailed instructions I will be reading them with determination! You have taken time out of your day to help someone and I thank you... :D Now to be extremely cheeky: Any ideas as to how I can stop the fadeIn and fadeOut that is called on the window width < 960 when the user expands the window??? \$\endgroup\$ – vimes1984 Mar 29 '13 at 15:29
  • \$\begingroup\$ Check out that plugin I put in there. That should solve your problems. Just read the docs and see the examples. If you have questions I can help answer them. \$\endgroup\$ – Jonny Sooter Mar 29 '13 at 15:31
  • \$\begingroup\$ thanks @Jonny I have implemented your fixes and learnt a lot :D I do have one question: The .toggle i am using is due to an element fading out when one div expands and the screen width is beneath 960px's you can see a copy of the script hear: link and a copy of the working script on the root of that server. bascialy I want to unbind the fadin fadeout once the screen returns to over 960px's I posed the question here link thanks :D \$\endgroup\$ – vimes1984 Mar 29 '13 at 19:53
  • \$\begingroup\$ What do you mean by "stop". Do you want it to freeze in the middle of the animation, or to become completely visible or invisible? \$\endgroup\$ – Jonny Sooter Mar 30 '13 at 2:50
  • 2
    \$\begingroup\$ When in doubt about any method, just read the jQuery documentation. For resize it's this: api.jquery.com/resize \$\endgroup\$ – Jonny Sooter Apr 3 '13 at 18:32
4
\$\begingroup\$

One simple thing you could do is to use jQuery as $ inside your ready function.

Since the ready function takes a parameter to jQuery, you could capture that as $ for the duration of the ready function:

jQuery(document).ready(function($){

  if ( $(window).width() < 960) {
  ...

That would likely help improve the readability of the code.

For more info on this capability, you can reference jQuery docs on the ready function, which call this nice feature out in the "Aliasing the jQuery Namespace" section - specifically intended for use in $.noConflict() mode.

Also, I think you want to switch the ready call to reference document directly, instead of the string "document":

jQuery(document).ready(...

instead of

jQuery("document").ready(...

You might also consider creating this CSS rule:

#sb-container div.clicked {
  transform: rotate(0deg);
  -webkit-transform: rotate(0deg);
  -ms-transform: rotate(0deg);
  -moz-transform: rotate(0deg);
  -o-transform: rotate(0deg);
}

You can then just apply (or remove) the clicked class to that div instead of applying all the CSS directly in javascript. (Separates out the styling to the CSS file from the behaviour in the JS file.)

$('.menu-button').click(function(){
   $('#sb-container div').addClass('clicked');
   $('.main').animate({'right': '1%'}, 'slow');
});

You could also consider using fadeToggle instead of toggle with two fade functions.

\$\endgroup\$
  • \$\begingroup\$ all implemented thank you for your time and patiance, I was running jQuery in no conflict I just didn't know you could initilize the $ again in the document ready function. \$\endgroup\$ – vimes1984 Mar 29 '13 at 15:19
  • \$\begingroup\$ @vimes1984 would you mind up-voting this answer since you implemented it? Thanks! \$\endgroup\$ – Eric S Mar 29 '13 at 18:07
  • \$\begingroup\$ I tried but i can't I have to have at least 15 rep to upvote answers srry... \$\endgroup\$ – vimes1984 Mar 29 '13 at 19:46
2
\$\begingroup\$

For some reason i can't find the comment button, hence a answer.

Jonny Sooter's advice covers most, a extra i would add is to namespace your events.

$('selector').on('click.namespace', handler);

This kinda acts like a identifier, maybe you want to disable your events in a certain app state. By using your namespace you can target these specific events and only remove these specific events instead of removing all click events from that element.

$('your-selector').off('click.namespace');

By using a 'anomonous self invoking function' as Jonny covered earlier you can do what's called global import. What this basicly means is that you import variables into your closure and map/alias it to whatever you want.

;(function (Alias) {
// Now within this closure jQuery is mapped/aliased to the variable Alias. Another benefit of this is that this can enhance scope chain lookups, and create a private state for your local variables.
}(jQuery));

// Difference in scope chain lookups.
var a = 'something';
;(function (window, document) {
    document.location.href = a;
}(this, this.document));

var a = 'something';
;(function (importedVar, window, document) {
    document.location.href = importedVar;
}(a, this, this.document));

In the scope chain example the first part is slower then the last part, this is because how javascript works, it will check it's local scope first to see if it has a variable a, if it can't find it it goes up a context(nested functions) to see if its there, this will go on all the way up to the global window object, if it cant find it it will throw. Thats why you should avoid using global variables as much as possible, eg: window.something or using somthing without a var keyword before it.

In the second example we import that variable into our closure, so basicly it has a local reference. This goes alot deeper though but i'll keep it simple. :)

A very good read can be found here http://www.adequatelygood.com/JavaScript-Module-Pattern-In-Depth.html this is a pattern i use heavily.

And another very good vid from Paul Irish wit a few tips and tricks. http://paulirish.com/2010/10-things-i-learned-from-the-jquery-source/

\$\endgroup\$

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.