current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 3 down vote favorite

I have a pan control that look like this:

pan control

Pretty standard Google Mapsish controls, IMO.

When the user clicks in the top 3rd of the control, it should pan up (deltaY = +1); right third means it should pan east (deltaX = -1); Down is deltaY = -1, and West is deltaX = +1. The control is 50px, square. I have working code, but I believe it can be written much more elegantly than this using a single (if discontinuos) mathematical function. As always, I appreciate your feedback.

$(control).click(function (evt) {
    var clickPointX = (evt.layerX || evt.offsetX),
        clickPointY = (evt.layerY || evt.offsetY),
        deltaX,
        deltaY;

    // control is 50x50 px.
    // Click in the top third, get a positive 'Y' value.
    // Click in the left third, get a positive 'X' value.
    // Click in the middle, get zero.
    if (clickPointX < 16) {
        deltaX = +1;
    } else if (clickPointX > 34) {
        deltaX = -1;
    } else {
        deltaX = 0;
    }

    if (clickPointY < 16) {
        deltaY = +1;
    } else if (clickPointY > 34) {
        deltaY = -1;
    } else {
        deltaY = 0;
    }
    map.panDirection(deltaX, deltaY);
});
share|improve this question

3 Answers 3

up vote 3 down vote accepted

A few improvements:

  • replace magic numbers
  • extract repetition into a function
  • dynamically calculate dimensions

Example code here.

share|improve this answer
    
That is definitely DRYer. Thanks! –  kojiro Dec 16 '11 at 4:37
up vote 0 down vote

Probably more of a personal preference but i find three sections to an if hard to read.

I would set your Delta values to 0 initially

var deltaX = 0;

then you can remove:

} else {
    deltaX = 0;
}

and you have three less lines to read. Not a real performance improvement or anything though.

share|improve this answer
up vote 1 down vote

Well, if you want less repetition, you can do this. But I don't think there's anything wrong with your approach (which is more efficient incidentally).

var pan = function(clickx, clicky){
    var delta = function(val){
        return val < 16 ? 1 :
               val > 34 ? -1 :
               0;
    };
    return {dx: delta(clickx), dy: delta(clicky)};
};

I do think you should separate out the click handling and the panning algorithm into separate functions unless efficiency then becomes an issue.

share|improve this answer
    
You're right. I will probably do that, I just wanted to see if this could be made into a simple discontinuous mathematical function first. –  kojiro Dec 16 '11 at 4:38
1  
What I've written is a simple discontinuous mathematical function :-) –  mintsauce Dec 20 '11 at 2:35

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.