8
\$\begingroup\$

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');
        }
    });
});
\$\endgroup\$
  • \$\begingroup\$ I'm assuming $ stands for jQuery? \$\endgroup\$ – John Dvorak Mar 30 '13 at 14:32
  • \$\begingroup\$ @JanDvorak yea it is \$\endgroup\$ – ny95 Mar 30 '13 at 16:00
5
\$\begingroup\$

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]);
    });
});
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hm, i never thought about doing it like the last example you wrote, you said this will make the performance better? \$\endgroup\$ – ny95 Mar 30 '13 at 15:52
  • \$\begingroup\$ I would use +sign_in instead of sign_in?1:0 and +!sign_in instead of sign_in?0:1 \$\endgroup\$ – John Dvorak Mar 30 '13 at 16:02
  • \$\begingroup\$ @CristianRivera Sorry, I should've been more clear. It makes performance slightly worse since you call $ every time the event executes. \$\endgroup\$ – p.s.w.g Mar 30 '13 at 16:19
  • 1
    \$\begingroup\$ @JanDvorak good point. Personally I don't like that trick; I feel it harms readability. \$\endgroup\$ – p.s.w.g Mar 30 '13 at 16:21
  • \$\begingroup\$ 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? \$\endgroup\$ – ny95 Mar 30 '13 at 16:22
4
\$\begingroup\$

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');
      }
    });
});
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for this i am using your code for the index specifically with the other code from above \$\endgroup\$ – ny95 Apr 4 '13 at 16:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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