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.

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);
    }
 };
share|improve this question

migrated from stackoverflow.com Sep 11 '12 at 12:59

This question came from our site for professional and enthusiast programmers.

2  
That is actually reasonably compact, straightforward code. Which part were you hoping to simplify, exactly? One thought: if the click handler is what you're focusing on, you can always declare the function independently and assign it by variable name, rather than reproducing it inline repeatedly. –  Kevin Nielsen Sep 10 '12 at 16:38
    
The part of code I'm hoping to simplify is the click handler (starting from var current = null; to the end of the code). The problem I'm having is trying to rebuild the click handler code - or even calling the function to be used elsewhere. –  user1619342 Sep 11 '12 at 9:04
    
@MarkThomas I'll try codereview too, thanks. –  user1619342 Sep 11 '12 at 9:06
    
You could user Paper.set and make an array of elements. I wouldn't add it to the context as class property. And btw, you can add 'cursor': 'pointer' to your attr. And at last: Don't use for..in, because it's very vulnerable, try Google :) –  Dan Lee Sep 11 '12 at 9:41
    
Great, thanks Dan Lee. I'll look into those more. Google to the rescue! :) –  user1619342 Sep 13 '12 at 9:51

2 Answers 2

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.

share|improve this answer

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.

share|improve this answer
    
I see absolutely no problem with using for... in to enumerate object properties, Google did not help. The original click logic was idiomatic JavaScript, not sure why you changed it. Also, OP doesn't use jQuery ([0] comes from Raphaël). –  Dan Jan 7 at 0:55

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.