How can I make this jQuery more succinct?

Question

I'm fairly new to jQuery and would like some pointers on my following block of jQuery code, I'm interested in writing better code.

Basically, the code just slides content in from the left when you click on one of two menu items and then on a further click slides it out. I guess I'm trying to combine it into one code block rather than two functions.

$('#menu-item-91').click(function(e) {
    e.preventDefault();
    //console.log("Clicked");
    //$('.about').css('display','inherit').slideDown(3000);
    var sliding = $('.sliding');
    var sliding1 = $('.sliding1');

    if( sliding.css('display') == "none" && sliding1.css('display') == "none" ) { //show it
        sliding.fadeIn(0);
        sliding.animate({"left": 0},800);
    }else if ( sliding.css('display') == "none" && sliding1.css('display') == "block" ) {

        sliding1.animate({"left": -300},500 );
        sliding1.delay(500).fadeOut(0);
        sliding.fadeIn(0);
        sliding.animate({"left": 0},800);
    } else {

        sliding.animate({"left": -300},500 );
        sliding.delay(500).fadeOut(0);
    }

});

$('#menu-item-17').click(function(e) {
    e.preventDefault();
    //console.log("Clicked");
    //$('.about').css('display','inherit').slideDown(3000);
    var sliding = $('.sliding1');
    var sliding1 = $('.sliding');

    if( sliding.css('display') == "none" && sliding1.css('display') == "none" ) { //show it
        sliding.fadeIn(0);
        sliding.animate({"left": 0},800);
    }else if ( sliding.css('display') == "none" && sliding1.css('display') == "block" ) {

        sliding1.animate({"left": -300},500 );
        sliding1.delay(500).fadeOut(0);
        sliding.fadeIn(0);
        sliding.animate({"left": 0},800);
    } else {

        sliding.animate({"left": -300},500 );
        sliding.delay(500).fadeOut(0);
    }

});

That's it! This is my first post so I hope I haven't broke any rules and someone out there can help me be better!

Regards Tom

EDIT

Thanks Guys! Didn't want to mark an answer just yet as I think I probably missed out a bit of detail that's relevant for some of the answers.

The two classes .sliding and .sliding1 are added to separate divs - one called portfolio and one called about.

So the idea is that if you click the about menu item the about div slides in, then if you were to click the portfolio menu item then the about would slide out and portfolio would slide in. But if no items were displayed then the item you clicked would slide in.

Hope this clarifies things, some of the answers just slide in the same content when either menu item was clicked.

Thanks!

Answer 1

There's a few things you could do here:

• Make use of jQuery's :visible selector instead of checking for css display value of 'none'
• Chain your methods on the same element together on one line rather than adding them on separate lines
• It looks like the only differences between the two click functions are the elements they're called on, so instead maybe use a single click function with a comma separated list of selectors

Here's an adapted version of your code:

Edit I've added a function that you can call and pass in the selectors for the two sliding variables based on comment feedback from Ricardo below:

$("#menu-item-17").click(function(e) {
  e.preventDefault();
  handleSlider($(".sliding1"), $(".sliding"));
});

$("#menu-item-91").click(function(e) {
  e.preventDefault();
  handleSlider($(".sliding"), $(".sliding1"));
});

function handleSlider(sliding, sliding1)
{
  if (!sliding.is(":visible") && !sliding1.is(":visible"))  //show it
  {
    sliding.fadeIn(0).animate({"left": 0},800);
  } 
  else if (!sliding.is(":visible") && sliding1.is(":visible")) 
  {
    sliding1.animate({"left": -300},500 ).delay(500).fadeOut(0);
    sliding.fadeIn(0).animate({"left": 0},800);
  } else {
    sliding.animate({"left": -300},500 ).delay(500).fadeOut(0);
  }
}

Answer 2

The first problem I see here is that most of your code is duplicate, and that has nothing to do with jQuery itself, it's a DRY issue. Make use of both :visible and :hidden pseudo classes that jQuery provides and wrap your common code in one place and also make use of chaining to avoid repeatedly using the variable name over and over; last but not least, use $ prefix on jQuery object variables, so they can instantly stand out as such:

var $sliding1,
    $sliding2;

function doSlide($item1, $item2) {
    if ($item1.is(':hidden') && $item2.is(':hidden')) { //show it
        $item1.fadeIn(0)
             .animate({"left": 0}, 800);
    }
    else if ($item1.is(':hidden') && $item2.is(':visible')) {
        $item2.animate({"left": -300}, 500)
             .delay(500)
             .fadeOut(0);
        $item1.fadeIn(0)
             .animate({"left": 0}, 800);
    }
    else {
        $item1.animate({"left": -300}, 500)
             .delay(500)
             .fadeOut(0);
    }
}

function fetchSliders() {
  $sliding1 = $sliding1 || $('.sliding');
  $sliding2 = $sliding2 || $('.sliding1');
}

$('#menu-item-91').click(function(e) {
    e.preventDefault();
    fetchSliders();
    doSlide($sliding1, $sliding2);
});

$('#menu-item-17').click(function(e) {
    e.preventDefault();
    fetchSliders();
    doSlide($sliding2, $sliding1);
});

Answer 3

        function slideThis(sliding,sliding1)
        {
             if(sliding.is(":visible"))
             sliding.animate({"left": -300},500 ).delay(500).fadeOut(0);

             else 
             {
                 if(sliding1.is(":visible"))
                 sliding1.animate({"left": -300},500 ).delay(500).fadeOut(0);

                 sliding.fadeIn(0).animate({"left": 0},800);
             }    

        }


    var sliding = $('.sliding');
    var sliding1 = $('.sliding1');

    $('#menu-item-91').click(function(e) {
        e.preventDefault();
        slideThis(sliding,sliding1); //notice the changes in parameter name
    });

    $('#menu-item-17').click(function(e) {
        e.preventDefault();
        slideThis(sliding1,sliding); //notice the changes in parameter name
    });

Answer 4

using multple id selectors, but you could use a class instead.

 var sliding = $('.sliding');
 var sliding1 = $('.sliding1');

$('#menu-item-91, #menu-item-17').click(function(e) {
    e.preventDefault();
    if (sliding.css('display') === "none" && sliding1.css('display') === "none" ) { //show it`enter code here`
        sliding.fadeIn(0)
               .animate({"left": 0},800);
    }else if (sliding.css('display') === "none" && sliding1.css('display') === "block" ) {
        sliding1.animate({"left": -300},500 )
                .delay(500)
                .fadeOut(0);
        sliding.fadeIn(0)
               .animate({"left": 0}, 800);
    } else {
        sliding.animate({"left": -300},500)
               .delay(500).fadeOut(0);
    }
}