0
\$\begingroup\$

I have built a JavaScript PNG sequence engine that allows for playing, pausing and stopping and a time change event. I am asking for genuine opinions if the code is actually good. Its ONLY for the iDevice range (not for desktop browsers or anything) and here is the code and a link to the gist if you wanted to edit it for examples.

Hope its as good as I think it is!

/**
 * @param slides ~ Array (ARRAY) of image locations
 * @param duration ~ Number - in milliseconds how long to run for
 * @param element ~ jQuery object - The <img> tag to change the src of.
 * @returns void
 * 
 * @example
 *  <img src="#" id="sequence" data-loop="true" data-autoplay="true" />
 *  <script>var vid = new PNGSequence(tiffanyCrystal, 4, $('#sequence'));</script>
 */
var PNGSequence = function (slides, duration, element) {
    this.slides = slides;
    this.NSlides = slides.length;
    this.duration = duration;
    this.interval = (this.duration * 1000) / this.NSlides;
    this.element = element;
    this.index = 0;
    this.loop = this.element.data('loop') == true;
    this.autoplay = this.element.data('autoplay') == true;

    var inter;

    this.interGo = function () {
        var self = this;
        return setTimeout(function () {
            // Update the image
            element.attr({
                "src": slides[self.index]
            });

            // Increase the index
            self.index++;

            // Call the onTimeChange event
            if (self.hasOwnProperty('onTimeChange')) {
                if (self.onTimeChange) {
                    self.onTimeChange.call(self, self.index, self.NSlides);
                }
            }

            // Check if its the end of the animation
            if (self.index > self.NSlides) {
                // If we're not looping, clear the interval
                if (!self.loop) {
                    clearTimout(inter);
                } else {
                    // If we're looping then start the loop from the beginning 
                    self.index = 0;
                    self.play();
                }
                return;
            }

            //Fire the timeout again
            self.interGo();
        }, this.interval);
    };

    /**
     * Pauses the sequence
     */
    this.pause = function () {
        clearTimout(inter);
        return this.index;
    };

    /**
     * Plays the sequence from last known point
     */
    this.play = function () {
        return inter = this.interGo();
    };

    /**
     * Stops the sequence and sets the pointer
     * at the beginning again
     */
    this.stop = function () {
        clearTimout(inter);
        this.index = 0;
        return 0;
    };

    this.onTimeChange = function () {
        return this;
    };

    if (this.autoplay) {
        inter = this.interGo();
    }

    return this;
};

And here is the link to the gist https://gist.github.com/3360011

\$\endgroup\$
1
  • 1
    \$\begingroup\$ As per the FAQ, you should include the code in the question. If the gist is changed or deleted the answers no longer match the question they were answering... \$\endgroup\$
    – ANeves
    Commented Aug 15, 2012 at 15:21

2 Answers 2

2
\$\begingroup\$

I would suggest a few minor changes:

  1. Use setTimeout instead of setInterval to ensure that callbacks don't stack up if they can't be serviced on time. This implies re-invoking this.interGo at the end of the callback to re-trigger the timer.

  2. Only declare and use self in the functions that need it, to avoid traversing the scope chain unnecessarily.

  3. Remove over-reliance on the Number constructor.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks man! Only took 30 minutes to build, hadn't noticed I was using self. instead of this. in the functions IN scope lol. I've updated the gist :) gist.github.com/3360011 \$\endgroup\$ Commented Aug 15, 2012 at 14:04
  • 1
    \$\begingroup\$ In your new code I'd move the var self = this just inside the interGo function. It'll stop the interpreter from trying to build the closure around the whole function instead of just the inner function. Also, move your end-of-loop test to the end of the function, where you can merge together the question of resetting the index or firing again. \$\endgroup\$
    – Alnitak
    Commented Aug 15, 2012 at 14:10
  • \$\begingroup\$ I've updated the gist again, I didn't quite understand what you meant by "merge together the question of resetting the index or firing again" \$\endgroup\$ Commented Aug 15, 2012 at 14:42
  • \$\begingroup\$ I mean that the last block just needs to say if (self.looping && self.index > self.NSlides) { index = 0; } and then just call self.interGo. There's no need for a clearTimeout. See jsfiddle.net/alnitak/mVF75 \$\endgroup\$
    – Alnitak
    Commented Aug 15, 2012 at 14:48
  • \$\begingroup\$ oops, that's not quite right. \$\endgroup\$
    – Alnitak
    Commented Aug 15, 2012 at 16:19
1
\$\begingroup\$

Do you expect this to be called multiple times per page? If so, I would consider attaching your functions to the prototype, that way they're not instantiated every time a your object is new'd up.

If this is primarily for use with jQuery, why not bundle it into a plugin? You can still keep your object as it is, just follow the kind of pattern used by twitter in the bootstrap plugins. Off the top of my head, something like this perhaps?

$.fn.pngSequence = function(options) {
    return this.each(function() {
        var $this = $(this),
            sequence = new PNGSequence(options.slides, options.duration, $this);

        //i like storuing the instance in the data collection
        //so consumer can interact with it later
        $this.data("pngSequence", sequence);

    });
};

Which could then be utilised as so. Note that it's hooked in declaratively using a data-* attribute :)

$("img[data-provider='pngSequence']").pngSequence({
    slides : tiffanyCrystal,
    duration : 4
});
\$\endgroup\$
1
  • \$\begingroup\$ Actually it turned out, no amount of optimisation ran smoothly on all the iPads, they all seemed to fall over or lag depending on different optimisations so this was ditched. Be ok for web use though, I like your idea of using it in a jQuery plugin and storing it in data. \$\endgroup\$ Commented Sep 14, 2012 at 13:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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