Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 7 down vote favorite
4

is there anyway I can simplify this piece of javascript? It seems inefficient.

$(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');
        }
    });
});
share|improve this question
I'm assuming $ stands for jQuery? – Jan Dvorak Mar 30 at 14:32
@JanDvorak yea it is – Cristian Rivera Mar 30 at 16:00

2 Answers

up vote 5 down vote accepted

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]);
    });
});
share|improve this answer
Hm, i never thought about doing it like the last example you wrote, you said this will make the performance better? – Cristian Rivera Mar 30 at 15:52
I would use +sign_in instead of sign_in?1:0 and +!sign_in instead of sign_in?0:1 – Jan Dvorak Mar 30 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 at 16:19
1  
@JanDvorak good point. Personally I don't like that trick; I feel it harms readability. – p.s.w.g Mar 30 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? – Cristian Rivera Mar 30 at 16:22
show 1 more comment
up vote 4 down vote

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');
      }
    });
});
share|improve this answer
Thanks for this i am using your code for the index specifically with the other code from above – Cristian Rivera Apr 4 at 16:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.