4
\$\begingroup\$

Note: This was originally posted at SO, and as per the recommendation moving to this form.

In my effort to learn JS on the backdrop of SVG development, I got lot of help from SO so far. The answer to previous query was the best to understand closure; its usage and caveats in very short time. While continuing on the same line, I was trying to breakdown the below function so that it is more readable and easy to expand. It was part of an answer given by @Phrogz. I could not think a way to keep the onmove closure outside the loop.

If someone can help me with a pseudo code/skeleton of how can it be done without too many variable to retain the state (like button click state while moving etc), it would clarify many of my doubts regarding how the closure can be assigned etc. You may also break the function into 3 event handlers (mousedown, mousemove, mouseup). I could not think a way to do this while keeping the context (mousemove only triggers when the mouse is down, as it is within the closure).

I use jQuery typically in my projects, so if that helps to make the above code more maintainable, that would help too.

for (var a=svg.querySelectorAll('.drag'),i=0,len=a.length;i<len;++i){
        (function(el){
            var onmove; // make inner closure available for unregistration
            el.addEventListener('mousedown',function(e){
                el.parentNode.appendChild(el); // move to top
                var x = el.tagName=='circle' ? 'cx' : 'x';
                var y = el.tagName=='circle' ? 'cy' : 'y';
                var mouseStart   = cursorPoint(e);
                var elementStart = { x:el[x].animVal.value, y:el[y].animVal.value };
                onmove = function(e){
                    var current = cursorPoint(e);
                    pt.x = current.x - mouseStart.x;
                    pt.y = current.y - mouseStart.y;
                    console.log("pt.x:" + pt.x + "pt.y:" + pt.y);
                    var m = el.getTransformToElement(svg).inverse();
                    m.e = m.f = 0;
                    pt = pt.matrixTransform(m);
                    el.setAttribute(x,elementStart.x+pt.x);
                    el.setAttribute(y,elementStart.y+pt.y);
                    var dragEvent = document.createEvent("Event");
                    dragEvent.initEvent("dragged", true, true);
                    el.dispatchEvent(dragEvent);
                };
                document.body.addEventListener('mousemove',onmove,false);
            },false);
            document.body.addEventListener('mouseup',function(){
                document.body.removeEventListener('mousemove',onmove,false);
            },false);
        })(a[i]);
    }
\$\endgroup\$
2
\$\begingroup\$
  1. Add more spaces.
  2. Use more named functions. (wrap the whole thing in an anonimous wrapper if you're worried about name space conflicts).
  3. When you do use lamdas give them names (it makes the code easier to read and you don't get showing up in the error log.
  4. Declare variables in higher functions to take advantage of closures. eg x, y, elementStart and mouseStart.
  5. camelCase.
  6. Use === unless you explicitly want to force casting.
  7. Don't make functions in a loop.

http://codr.cc/s/03c0d0af/js

\$\endgroup\$
0
\$\begingroup\$

The function in a loop thing:

function innerStuff(el) {
        var onmove; // make inner closure available for unregistration
        el.addEventListener('mousedown',function(e){
        // ... etc etc
    }
}

for (var a=svg.querySelectorAll('.drag'),i=0,len=a.length;i<len;++i){
    innerStuff(a[i]);
}

This will mean the variables are inside the innerStuff() function and you are not creating a new function every time. (don't use that name ... make your own descriptive one.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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