Simplify Raphael JS code: display text in DIV on click event

Question

I've been working with the RaphaelJS 'australia map' example that displays text in a DIV when a state is clicked on. However I'm having difficulty working with it to create different events and functions.

I want to be able to duplicate the 'display text' function for two different element onClick events (i.e. have two different buttons (elements) that do the same thing). But the current code is a bit too complicated for me. Can somebody please help me simplify the code that displays the text in a DIV when an element is clicked on.

Any help would be much appreciated - this is what I'm currently working with jsfiddle.

 window.onload = function () {
    var R = Raphael("paper", 532, 500);
    var attr = {
        fill: "#0B0",
        stroke: "#fff",
        "stroke-width": 3,
    };
    var ctx = {};
    ctx.btn_1 = R.path("M 180,130 L 340,130, L 392,212, L 128,212 z").attr(attr);
    ctx.btn_2 = R.path("M 128,212 L 392,212, L 435,280, L 85,280 z").attr(attr);

    var current = null;
    for (var state in ctx) {
        (function (st, state) {
            st[0].style.cursor = "pointer";
            st[0].onmousedown = function () {
                current && (document.getElementById(current).style.display = "");
                st.toFront();
                R.safari();
                document.getElementById(state).style.display = "block";
                current = state;
            };
        })(ctx[state], state);
    }
 };

Answer 1

I agree with tiffon it's better to extract a function, but I don't agree with the rest of their edits, so here's what I would have done (I also kept the style intact).

var initPath = (function () {
    var currentEl;

    function init(el, path) {
        path.attr({
            fill: "#0B0",
            stroke: "#fff",
            "stroke-width": 3,
        });

        path[0].style.cursor = "pointer";
        path[0].onmousedown = function () {
            currentEl && (currentEl.style.display = "");

            path.toFront();
            path.paper.safari();

            el.style.display = "block";
            currentEl = el;
        };
    }

    return init;
})();

window.onload = function () {
    var R = Raphael("paper", 532, 500);
    var paths = {
        'btn_1': R.path("M 180,130 L 340,130, L 392,212, L 128,212 z"),
        'btn_2': R.path("M 128,212 L 392,212, L 435,280, L 85,280 z")
    };

    for (var id in paths) {
        initPath(document.getElementById(id), paths[id]);
    }
};

Firstly, I think st and state were confusing names, so I changed them to be path and id, respectively, and then changed id to el to avoid additional lookups.

I separated the infrastructure (current and initPath) from the usage (R, paths and the loop).

I also think an object literal looks better than a series of assignments, so I changed that.

Finally, I like it better if I move initPath and currentEl into a closure of their own—after all, they don't depend on window.onload anymore.

I left the inner init a named function so it is named in a stack trace, if there is an error.

Answer 2

I gave it a shot.

window.onload = function () {
    var R = Raphael("paper", 532, 500),
        attrs = {
            fill: "#0B0",
            stroke: "#fff",
            "stroke-width": 3,
            cursor: "pointer"
        },
        paths = [
            {id: 'btn_1', p:"M 180,130 L 340,130, L 392,212, L 128,212 z"},
            {id: 'btn_2', p:"M 128,212 L 392,212, L 435,280, L 85,280 z"}
        ],
        current,
        max,
        i;

    for (i = 0, max = paths.length; i < max; i++) {
        initPath(paths[i].p, attrs, paths[i].id);
    }

    function initPath(pathStr, attrs, targetId) {

        var path = R.path(pathStr).attr(attrs),
            txElm = document.getElementById(targetId);

        path.click(function (e) {
             if (current) {
                 if (current === txElm) {
                     return;
                 }
                 current.style.display = '';
             }
             current = txElm;
             current.style.display = 'block';
             this.toFront();
             R.safari();
        });
     }
 }​

Integrated Dan Lee's comments and defined a function instead of making a new closure in each loop iteration.