How can I not repeat this code in jquery?

Question

Hi there, I am repeating an IF ELSE statement like so

 $('#accountLoginButton').click(function() {




  if($('#topSubscribe').is(":visible")) {

   $('#topSubscribe').slideUp(function(){


   if ($('#topLogin').is(":hidden"))
   {
     $('#topLogin').slideDown("fast");
   } else {
     $('#topLogin').slideUp("fast");
   }

   });

  } else {

    if ($('#topLogin').is(":hidden"))
   {
     $('#topLogin').slideDown("fast");
   } else {
     $('#topLogin').slideUp("fast");
   }

  }




 });



 $('#subscribeTopButton').click(function() {

  if($('#topLogin').is(":visible")) {

   $('#topLogin').slideUp(function(){


   if ($('#topSubscribe').is(":hidden"))
   {
     $('#topSubscribe').slideDown("fast");
   } else {
     $('#topSubscribe').slideUp("fast");
   }

   });

  } else {

    if ($('#topSubscribe').is(":hidden"))
   {
     $('#topSubscribe').slideDown("fast");
   } else {
     $('#topSubscribe').slideUp("fast");
   }

  }


 });

Basically 2 buttons operating like tabs to show/hide stuff.

As you can see I literally have the same code being repeated in a few different ways a couple times. I have a feeling i could somehow get this down to a few lines of code but my javascript understanding is a bit shady in general.

How do I trim this down the most?

Answer 1

You can put that code-block into a function and call it in both locations as needed.

Answer 2

Try refactoring it to smaller functions, then you should be able to see duplication more easily.

Answer 3

You could use slideToggle instead of checking for visibility and then using slideDown or slideUp.

You can replace:

if ($('#topLogin').is(":hidden"))
{
     $('#topLogin').slideDown("fast");
} else {
     $('#topLogin').slideUp("fast");
}

with:

$('#topLogin').slideToggle("fast");

This should allow you to get rid of a lot of repetition in your code.

Answer 4

You're merely running to much code inside your if block. Try this instead:

$('#accountLoginButton').click(function() {
    if($('#topSubscribe').is(":visible")) {
        $('#topSubscribe').slideUp();
    }
    $('#topLogin').slideToggle('fast');
}

Or you can, as several others have proposed, factor out the repeated code into a separate function:

$('#accountLoginButton').click(function() {
    if($('#topSubscribe').is(":visible")) {
        $('#topSubscribe').slideUp(function(){
            $('#topLogin').slideToggle('fast');
        });
    } else {
        $('#topLogin').slideToggle('fast');
    }
});

EDIT: Using .slideToggle() instead of the if block.

Answer 5

You could try something like this:

function handleVisibilityOf( el, otherEl ) {
  if( el.is(':visible') ) {
    el.slideUp( function() {
      otherEl.slideToggle();
    } );
  }
  else {
    otherEl.slideToggle();
  }
}

$('#accountLoginButton').click( function() {
  handleVisibilityOf( $('#topSubscribe'), $('#topLogin') );
} );

$('#subscribeTopButton').click( function() {
  handleVisibilityOf( $('#topLogin'), $('#topSubscribe') );
} );

Answer 6

function slideBoth(elm1, elm2){
  if(elm1.is(":visible")){
    elm1.slideUp(function(){
      elm2.slideToggle("fast");
    }
  }else{
    elm2.slideToggle("fast");
  }
}


$('#accountLoginButton').click(function() {
  slideBoth($("#topSubscribe"), $("#topLogin"));
}



$('#subscribeTopButton').click(function() {
  slideBoth($("#topLogin"),$("#topSubscribe"));
}