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

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');
}
});
});

• I'm assuming $ stands for jQuery? – John Dvorak Mar 30 '13 at 14:32 • @JanDvorak yea it is – ny95 Mar 30 '13 at 16:00 ## 2 Answers You could do this: $(function () {
var $carousel =$('#carousel');
var $switch =$('#switch');
var $headerSubmit =$('#header, #submit');
var $submit =$('#submit');
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]); }); });  • Hm, i never thought about doing it like the last example you wrote, you said this will make the performance better? – ny95 Mar 30 '13 at 15:52 • I would use +sign_in instead of sign_in?1:0 and +!sign_in instead of sign_in?0:1 – John Dvorak Mar 30 '13 at 16:02 • @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 Mar 30 '13 at 16:19
• @JanDvorak good point. Personally I don't like that trick; I feel it harms readability. – p.s.w.g Mar 30 '13 at 16:21
• 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? – ny95 Mar 30 '13 at 16:22

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');

$carousel.on('slid', function() { var isSignIn =$carouselItems.index('.active') === 0;
$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');