HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Small animation with Raphael

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withanimationsmallraphael

Problem

I have a small animation with Raphael using a large amount of code. Is there any way to reduce it?

Demo

window.onload = function () {
    var elements = document.querySelectorAll('.colbr');
    for (i=0; i<elements.length; i++) {
        var loadSpinner = spinner(elements[i], 7, 12, 10, 3, "#000");
    }
};

function spinner(holderid, R1, R2, count, stroke_width, colour) {
    var sectorsCount = count || 12,
        // color = colour || "#000",
        color = "#000",
        width = stroke_width || 15,
        r1 = Math.min(R1, R2) || 35,
        r2 = Math.max(R1, R2) || 60,
        cx = r2 + width,
        cy = r2 + width,
        r = Raphael(holderid, r2 * 2 + width * 2, r2 * 2 + width * 2),

        sectors = [],
        opacity = [],
        beta = 2 * Math.PI / sectorsCount,

        pathParams = {stroke: color, "stroke-width": width, "stroke-linecap": "round"};
        Raphael.getColor.reset();
    for (var i = 0; i < sectorsCount; i++) {
        var alpha = beta * i - Math.PI / 2,
            cos = Math.cos(alpha),
            sin = Math.sin(alpha);
        opacity[i] = 1 / sectorsCount * i;
        sectors[i] = r.path([["M", cx + r1 * cos, cy + r1 * sin], ["L", cx + r2 * cos, cy + r2 * sin]]).attr(pathParams);
        if (color == "rainbow") {
            sectors[i].attr("stroke", Raphael.getColor());
        }
    }
    var tick;
    (function ticker() {
        opacity.unshift(opacity.pop());
        for (var i = 0; i < sectorsCount; i++) {
            sectors[i].attr("opacity", opacity[i]);
        }
        r.safari();
        tick = setTimeout(ticker, 1000 / sectorsCount);
    })();
    return function () {
        clearTimeout(tick);
        r.remove();
    };
}

Solution

I really like what you wrote there, though I am afraid at this point you ought to expand the code, not reduce it.

  • r, r1, r2, cx, cy, beta are all variables that need better ( more meaningful ) names



  • You need to indent your code, just hit TidyUp in your fiddle and you will see



  • This is not part of your core code, but you should not simply assign something to window.onload, you should use addEventListener



  • Your code had support for using custom colors, and then you took it out, if you really want to take that out, then you should also take out the parameter, the commented out code and the if (color == "rainbow"){ statement



  • in your var statement, you could simply do cy = cx



  • be consistent in your name, and use lowerCamelCase: stroke_width -> strokeWidth



  • You did not declare i with var



  • While straightforward, your code has no comments at all..

Context

StackExchange Code Review Q#44261, answer score: 2

Revisions (0)

No revisions yet.