Image/content slider

Question

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

Answer 1

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

Answer 2

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.