1
\$\begingroup\$

I've built an image/content slider using JavaScript and as I've added to it and built in new functionality I've found myself writing several if statements in order to achieve the required response.

I'm now going over it again and I want to refactor it to make it shorter (if possible) and to increase performance (again if possible).

Has anyone any ideas on how I can refactor this? I'm thinking along the line of switch statements, but I'm not sure if that's the right route to go down or whether it would actually improve anything.

/*
 * animate margin left and store current position
 */

slider.prototype.transition = function (position, offset) {
    //'use strict';

    this.position = position;

    this.offset = offset;
    // update new slide position if deeplinking

    this.slide_pos = parseInt(this.position) || this.slide_pos;

    //check slide no exists in dl_urls object
    //this.deep_link_slide_no in window ? this.deep_link_slide_no = 0 : this.deep_link_slide_no = deep_link_slide_no;
    if (window.hasOwnProperty(this.deep_link_slide_no)) {
        this.deep_link_slide_no = 0;
    } else {
        this.deep_link_slide_no = position;
    }

    if(this.position === 0 || this.slide_pos === 0) {

        if(supportCSSTransitions && this.options.hardware_accelerated) {

          if(this.options.centered) {
            this.container.css({
              'margin-left': - (0.5 * this.slideWidth),
              transition :  'margin 0.8s ease'
            });
          } else {
            this.container.css({
              'margin-left': this.offset || 0,
              transition :  'margin 0.8s ease'
            });
          }
        } else {

          if(this.options.centered) {
            this.container.velocity({
              'margin-left': - (0.5 * this.slideWidth),
            }, {
                duration: this.options.slide_delay,
                easing: this.options.easing
            });
          } else {
            this.container.velocity({
              'margin-left': this.offset || 0,
            }, {
                duration: this.options.slide_delay,
                easing: this.options.easing
            });
          }
        }

        this.slide_pos = 0;

    } else {

      if(supportCSSTransitions && this.options.hardware_accelerated) {
        //console.log ("css and hardware");

        if(this.options.centered) {

          this.container.css({
            'margin-left': - (this.slide_pos * this.slideWidth) - (0.5 * this.slideWidth),
            transition :  'margin 0.8s ease'
          });

        } else if (typeof this.offset != "undefined") {
          this.container.css({
              'margin-left': (((this.slide_pos * this.slideWidth) * -1) + this.offset),
              transition : 'margin 0.8s ease'
          });
        } else {
          this.container.css({
              'margin-left': ((this.slide_pos * this.slideWidth * -1)) || (this.current_margin + this.direction),
              transition : 'margin 0.8s ease'
          });
        }



      } else {

        if(this.options.centered) {
          this.container.velocity({
            'margin-left': - (this.slide_pos * this.slideWidth) - (0.5 * this.slideWidth),
          });
        } else if (typeof this.offset != "undefined") {
          this.container.velocity({
              'margin-left': (((this.slide_pos * this.slideWidth) * -1) + this.offset)
          });
        } else {
          this.container.velocity({
              'margin-left': ((this.slide_pos * this.slideWidth * -1)) || (this.current_margin + this.direction)
          });
        }
      }
    }

    // store and update margin each time
    this.current_margin = (this.current_margin + this.direction);

    // set button state
    this.set_button_state();

    // set active states
    this.set_active_states();
};
\$\endgroup\$
  • 1
    \$\begingroup\$ You have a considerable amount of repeated code in with this.container.css \$\endgroup\$ – james_womack Aug 3 '14 at 21:07
1
\$\begingroup\$

There are a few things I can spot in your code that seem off to me.

  • Your code could realy do with some more comments. You put the comments on the lines that seem quite obvious to me, but when you start calculating the new position things get a little more complicated and you don't comment any more. I believe it should be the other way around.

  • Animating margins is not realy such a good idea when it comes to performance. You are much better of using the cheaper transform: translate3d() as I tried to explain here. It should make your animation a lot smoother.

  • There is a lot of repetition in your code, which is never a good idea. You set that 'margin-left' on 10 different lines for example. Why not first calculate it, and then set it just once? If you decide, for example, to work with transforms somewhere in the future, you would have to update all 10 of those lines, while it could be just a single line. And it would make your code a lot shorter, which is always nice.

  • There are a lot of nested ifs in there, which does not improve the readability of your code. I try to never nest ifs. If you really have to, you are probably doing something wrong. Most often you can avoid this by splitting your code into multiple functions (single responsibility), working with early returns or just rethinking your logic.

  • When you assign just a single variable in an if / else block, you are better of using the ternary operator. It is more compact (which is always good in js) and I think it makes your code better readable.

I went ahead and tried to refactor your code with those tips in mind (not the translate, I leave that up to you to decide). Note that this code is not tested (it's a bit hard with only part of your plugin) but I believe it should do the same as your code does. It is a bit shorter and a lot better readable imo.

slider.prototype.transition = function(position, offset) {
    this.position = position;
    this.offset = offset;

    // update new slide position if deeplinking
    this.slide_pos = parseInt(this.position) || this.slide_pos;

    //check slide no exists in dl_urls object
    this.deep_link_slide_no =
        window.hasOwnProperty(this.deep_link_slide_no)
        ? 0
        : position;

    // -- calculate new margin left
    var marginLeft = 0;
    // position == 0
    if (this.position === 0 || this.slide_pos === 0) {
        marginLeft =
            this.options.centered
            ? -(0.5 * this.slideWidth)
            : this.offset || 0;
    }
    // position != 0
    else {
        marginLeft =
            this.options.centered
            ? -(this.slide_pos * this.slideWidth) - (0.5 * this.slideWidth)
            : this.offset == 'undefined'
                ? (((this.slide_pos * this.slideWidth) * -1) + this.offset)
                : ((this.slide_pos * this.slideWidth * -1)) || (this.current_margin + this.direction);
    }

    // -- animate
    // transitions supported
    if (supportCSSTransitions && this.options.hardware_accelerated) {
        this.container.css({
            'margin-left': marginLeft,
            transition: 'margin 0.8s ease'
        });
    }
    // transitions not supported
    else {
        this.container.velocity({
            'margin-left': marginLeft,
        }, {
            duration: this.options.slide_delay,
            easing: this.options.easing
        });
    }

    this.current_margin = (this.current_margin + this.direction);
    this.set_button_state();
    this.set_active_states();
};
\$\endgroup\$
  • \$\begingroup\$ Thanks for your comments, some really helpful points here! Thanks for the refactored code as well, i'll take your points on board and see what I can come up with \$\endgroup\$ – woolm110 Aug 4 '14 at 9:04
0
\$\begingroup\$

I wouldn't use switch statements, they are generally slower than if else so what you have here is fine.

You should swap the typeof this.offset != "undefined" for the faster this.offset !== undefined. Typeof is only needed if there is a chance the variable is not declared, which is not the case here.

You could also check which variables this.whatever are only used within this function and swap them for local variables. That should get you a marginal speed increase but more importantly: you won't clutter your instance with properties you don't really need.

Also... since your first comment is halfway there, consider using jsdoc and describe those two parameters.

\$\endgroup\$
  • \$\begingroup\$ I'm interested in your first statement that switch statements are generally slower. Do you have any sources? Some posts on SO even state that it's the other way around. In any case, I'd choose the more readable and more maintainable version. \$\endgroup\$ – ComFreek Aug 3 '14 at 20:59
  • \$\begingroup\$ Just search 'jsperf switch if' and draw your own conclutions. Although for some browsers switch can be faster, the difference is not that big. So this would probably be more a matter of readability and personal preference. \$\endgroup\$ – Sjeiti Aug 4 '14 at 6:40

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.