Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Here's a small module I've written that implements an animation which fades through the child elements in a container div on a continual loop.

I realise I could have done this way quicker using JQuery, but I wanted to write it from scratch in this case.

The module works well enough, but I'd be very grateful for any comments on the code.

(function() {

    function fade(e, duration, direction, /* 'in', 'out' */ easing, /* func */ complete /* callback */) {

        // Set default values for parameters
        if (easing === undefined) easing = linear;
        if (complete === undefined) complete = function(){};

        // initialise counter values
        var percentComplete = 0;
        var opacity = parseFloat(e.style.opacity);
        if (opacity < 0) opacity = 0;
        if (opacity > 1) opacity = 1;

        // initialise values that stay constant throughout execution
        var startOpacity = opacity;
        var framerate = 60;
        var startTime = (new Date()).getTime();

        function animate() {
            if (percentComplete >= 1.0) {
                return complete();
            }

            var elapsedTime = (new Date()).getTime() - startTime;
            percentComplete = elapsedTime / duration;

            var easing_options = {elapsedTime: elapsedTime, endValue: 1, totalDuration: duration}

            if (direction === "in") {
                easing_options.startValue = startOpacity;
                opacity = easing(easing_options);
            }
            else if (direction === 'out') {
                easing_options.startValue = 1 - startOpacity;
                opacity = 1 - easing(easing_options); 
            };
            e.style.opacity = opacity;
            setTimeout(animate, 1000/framerate);
        }

        setTimeout(animate, 1000/framerate);
    };

    function fadeIn(e, duration, easing, complete) {
        fade(e, duration, 'in', easing, complete);
    }

    function fadeOut(e, duration, easing, complete) {
        fade(e, duration, 'out', easing, complete);
    };

    // default easing function
    var linear = function(options) {
        var elapsedTime = options.elapsedTime;
        var startValue = options.startValue;
        var endValue = options.endValue;
        var totalDuration = options.totalDuration;

        var difference = endValue - startValue;

        var result = startValue + ((elapsedTime / totalDuration) * difference)

        return result
    };

    // Utility. Bidirectional circular iterator.
    function Cycle(arraylike) {
        this.values = arraylike;
        this.index = null;
    }
    Cycle.prototype._resetIndex = function() {
        this.index = this.index % this.values.length;
    };
    Cycle.prototype.next = function() {
        if (this.index === null) {
            this.index = 0;
        }
        else {
            this.index += 1;
            this._resetIndex();
        }
        return this.values[this.index]
    };
    Cycle.prototype.previous = function() {

        this.index = (this.index === null ? 0 : this.index) + (this.values.length - 1)
        this._resetIndex();
        return this.values[this.index];
    };
    Cycle.prototype.current = function() {
        return (this.index !== null ? this.values[this.index] : this.next())
    };


    // The main fader constructor. Fades through the child elements of a
    // container div on a continual loop.
    function Fader(options /* container, fadeLength, pauseLength */) {
        this.container = document.getElementById(options.container);
        var ch = this.container.children

        // Initialise opacity and position for child Elements of the
        // container div
        ch[0].style.opacity = 1;
        ch[0].style.position = "absolute";
        ch[0].style.top = "0px";
        ch[0].style.left = "0px";

        for (i = 1; i < ch.length; i++) {
            ch[i].style.opacity = 0;
            ch[i].style.position = "absolute";
            ch[i].style.top = "0px";
            ch[i].style.left = "0px";
        }

        this.slides = new Cycle(ch);
        this.fadeLength = options.fadeLength;
        this.pauseLength = options.pauseLength;
    }

    Fader.prototype.start = function() {
        var self = this;
        var fadeLength = this.fadeLength;
        var pauseLength = this.pauseLength;
        setTimeout(function() {
            fadeOut(self.slides.next(), fadeLength);
            fadeIn(self.slides.next(), fadeLength);
            self.timer = setInterval(function() {
                fadeOut(self.slides.current(), fadeLength);
                fadeIn(self.slides.next(), fadeLength);
            }, fadeLength + pauseLength)
        }, pauseLength);
    };

    Fader.prototype.stop = function() {
        clearInterval(this.timer);
        this.slides.previous();
    };

    // Exported object
    var exports = {
        fadeIn: fadeIn,
        fadeOut: fadeOut,
        noConflict: noConflict,
        Fader: Fader
    };

    // Mechanism to remove naming conflicts if another module exports
    // the name Fader
    var noConflictName = window['fadeLib'];
    function noConflict() {
        window['fadeLib'] = noConflictName;
        return exports;
    }

    // Export public interface
    window['fadeLib'] = exports;

})();

Here's a html snippet showing how the module is used.

<body>

    <div id="container" style="position:absolute; left:100px; width:500px;">
        <h1 style="color:red;">Hello world!</h1>
        <h1 style="color:red;">Goodbye!</h1>
        <h1 style="color:red;">Farewell!</h1>
    </div>

    <script src="faderLib.js"></script>
    <script>
        window.addEventListener("load", function() {
            var fader = new fadeLib.Fader({container: "container", fadeLength: 5000, pauseLength: 5000});
            fader.start();
        });
    </script>
</body>
share|improve this question
    
Available for fiddling @ jsfiddle.net/vudC2 (with a 2-second delay because I am impatient) –  Michael Paulukonis Jan 28 '13 at 19:27
    
does Cycle really need that much complexity? Why do you set style.top, style.left to 0px? –  Michael Paulukonis Jan 28 '13 at 20:33
    
Cycle.Next and Cycle.Previous use different idioms [is that the right word?] for basically the same code. Although Cycle.Previous makes an invariant call to _resetIndex –  Michael Paulukonis Jan 28 '13 at 20:56
    
I suppose its not strictly necessary for the purpose at hand. But it seemed like a generally useful pattern. I'm not sure I understand what you mean by "different idioms for the same code" in this case. Can you elaborate? –  samfrances Jan 28 '13 at 22:12
    
I added my thoughts about mixed idioms, below. –  Michael Paulukonis Jan 29 '13 at 2:44
add comment

1 Answer

up vote 2 down vote accepted

Small changes:

Cache the length, and set the invariants only once:

    // Initialise opacity and position for child Elements of the
    // container div
    for (var i = 0, l = ch.length; i < l; i++) {
        ch[i].style.opacity = 0;
        ch[i].style.position = "absolute";
        ch[i].style.top = "0px";
        ch[i].style.left = "0px";
    }

    // only first element is visible
    ch[0].style.opacity = 1;

Multiple coding idioms in the Cycle object: clear if...else in .next changes to a ternary op and an invariant call to .resetIndex in .previous, and change to _resetIndex to handle negative movement.

Cycle.prototype._resetIndex = function() {
    this.index = this.index % this.values.length;
};
Cycle.prototype.next = function() {
    if (this.index === null) {
        this.index = 0;
    }
    else {
        this.index += 1; 
        this._resetIndex();
    }
    return this.values[this.index]
};
Cycle.prototype.previous = function() {

    this.index = (this.index === null ? 0 : this.index) + (this.values.length - 1)
    this._resetIndex();
    return this.values[this.index];
};

change to (perhaps):

Cycle.prototype._resetIndex = function() {
    this.index = (this.index + this.values.length) % this.values.length;
};

Cycle.prototype.next = function() {
    if (this.index === null) {
        this.index = 0;
    }
    else {
        this.index++;
        this._resetIndex();
    }
    return this.values[this.index]
};

Cycle.prototype.previous = function() {

    if (this.index === null) {
        this.index = (this.values.length - 1);
    }
    else {
        this.index--;
        this._resetIndex();
    }

    return this.values[this.index];
};
share|improve this answer
    
Thanks. That clarifies your mixed idioms idioms comment. However, I believe there is a small error in the 'previous' method. For example, in a nodelist on length 6, where this.index starts at 3, calling previous would make this.index = 2 + (5 - 1) = 7. _resetIndex would then make this index 1, where we want index 2. I think the solution is just to say this.index += this.values.length - 1, and then call this,_resetIndex. –  samfrances Jan 29 '13 at 9:26
1  
I hadn't actually tried out the code, so fiddled with it a bit. Similar idioms can happen if we make a tweak to the _resetIndex method. Now next() and previous() are so similar it suggests some more refactoring might be possible (only the initial element on index === null and increment/decrement are different). –  Michael Paulukonis Jan 29 '13 at 17:10
add comment

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.