Switching some text labels when the first image of a carousel is active

Question

Is there any way I can simplify this inefficient piece of JavaScript?

$(function () {
 var $carousel = $('#carousel');
 var $switch = $('#switch');
 var $header = $('#header');
 var $submit = $('#submit');
 $carousel.bind('slid', function() {
 var index = $('#carousel .item').index($('#carousel .carousel-inner .active'));
 if (index == 0) {
 $header.text('Sign In');
 $switch.text('Sign Up');
 $submit.text('Sign In');
 $submit.attr('form', 'sign_in');
 } else {
 $header.text('Sign Up');
 $switch.text('Sign In');
 $submit.text('Sign Up');
 $submit.attr('form', 'sign_up');
 }
 });
});

• John Dvorak: I'm assuming $ stands for jQuery?
• ny95: @JanDvorak yea it is

Answer 1

You could do this:

$(function () {
 var $carousel = $('#carousel');
 var $switch = $('#switch');
 var $headerSubmit = $('#header, #submit');
 var $submit = $('#submit');
 var text_strings = [ 'Sign In', 'Sign Up' ];
 var form_strings = [ 'sign_in', 'sign_up' ];
 $carousel.bind('slid', function() {
 var index = $('#carousel .item').index($('#carousel .carousel-inner .active'));
 var sign_in = (index == 0);
 $headerSubmit.text(text_strings[sign_in ? 1 : 0]);
 $switch.text(text_strings[sign_in ? 0 : 1]);
 $submit.attr('form', form_strings[sign_in ? 1 : 0]);
 });
});

If you want to reduce the lines of code (with a slight negative impact to performance if this function gets called a lot), you could do this:

$(function () {
 var text_strings = [ 'Sign In', 'Sign Up' ];
 var form_strings = [ 'sign_in', 'sign_up' ];
 $('#carousel').bind('slid', function() {
 var index = $('#carousel .item').index($('#carousel .carousel-inner .active'));
 var sign_in = (index == 0);
 $('#header, #submit').text(text_strings[sign_in ? 1 : 0]);
 $('#switch').text(text_strings[sign_in ? 0 : 1]);
 $('#submit').attr('form', form_strings[sign_in ? 1 : 0]);
 });
});

• ny95: Hm, i never thought about doing it like the last example you wrote, you said this will make the performance better?
• John Dvorak: I would use +sign_in instead of sign_in?1:0 and +!sign_in instead of sign_in?0:1
• p.s.w.g: @CristianRivera Sorry, I should've been more clear. It makes performance slightly worse since you call $ every time the event executes.
• p.s.w.g: @JanDvorak good point. Personally I don't like that trick; I feel it harms readability.
• ny95: In the code above to counteract the calling of $ every time, can you load them into a var such as var $switch = $('#switch'); then call $switch so you dont have to query the object every time? In regards to Jan Dvorak, are there any performance differences?

Answer 2

It seems like you're using Bootstrap's Carousel? I've moved the two texts of the buttons to variables so that if you're using a minification tool (UglifyJs, Google's Closure Compiler etc), it'll help with minification.

I find using the index really reduces the readability of the code, so I've declared a variable isSignUp. This way, you reduce the cognitive load of the user reading the code. You don't have to remember which index is which after this declaration.

Since the .items are already on the page, you can cache their lookup with $carouselItems and then find the .active one with .index(). A better approach could be to check for a specific class rather than index.

Shorter code isn't always the best approach. I find that the current answer is difficult to read.

$(function () {
 var $carousel = $('#carousel');
 var $carouselItems = $carousel.find('.carousel-inner .item');
 var $switch = $('#switch');
 var $header = $('#header');
 var $submit = $('#submit');
 var SIGN_IN = 'Sign In';
 var SIGN_UP = 'Sign Up';
 $carousel.on('slid', function() {
 var isSignIn = $carouselItems.index('.active') === 0;
 if (isSignIn) {
 $header.text(SIGN_IN);
 $switch.text(SIGN_UP);
 $submit.text(SIGN_IN).attr('form', 'sign_in');
 }
 else {
 $header.text(SIGN_UP);
 $switch.text(SIGN_IN);
 $submit.text(SIGN_UP).attr('form', 'sign_up');
 }
 });
});

• ny95: Thanks for this i am using your code for the index specifically with the other code from above