3
\$\begingroup\$

How can I combine these if statements to make this a little cleaner and be less repetitive?

$(document).keydown(function(e){
    if (e.keyCode == 38) {
        if($html.hasClass('client-loading') || canAnim === false) {
            return false;
        }
        var $prevProj = $('.project-current').prev('.project');
        $prevProj.click();
    }

    if (e.keyCode == 40) {
        if($html.hasClass('client-loading') || canAnim === false) {
            return false;
        }
        var $nextProj = $('.project-current').next('.project');
        $nextProj.click();
    }

    // Prevent rapid clicking
    if ( e.keyCode == 38 || e.keycode == 40 ) {
        canAnim = false;

        setTimeout(function(){
            canAnim = true;
        },2000);
    }
});
\$\endgroup\$

6 Answers 6

2
\$\begingroup\$

Here's another one, just for the heck of it

$(document).keydown(function (e) {
  var current;

  // return early
  if( e.keyCode !== 38 && e.keyCode !== 40 ) {
    return false;
  }

  // again, return early
  if( $html.hasClass('client-loading') || canAnim === false ) {
    return false;
  }

  current = $('.project-current');

  if( e.keyCode === 38 ) {
    current.prev('.project').click();
  }

  if( e.keyCode === 40 ) {
    current.next('.project').click();
  }

  canAnim = false;
  setTimeout(function () { canAnim = true; }, 2000);
});
\$\endgroup\$
3
  • \$\begingroup\$ I believe canAnim and setTimeout should be done before the second return early. \$\endgroup\$ Jan 16, 2014 at 18:22
  • \$\begingroup\$ @SimonAndréForsberg Heh, took me a while to decipher this old code again. Anyway, I'm not so sure about your suggestion. As far as I can tell, the timeout should only be set if we've actually called click() on something (i.e. only if we make it past the 2 early returns). It's either-or: Either go next/prev and set a timeout, or do nothing at all. If setTimeout is called before the 2nd return, as you suggest, rapid key presses will keep setting new timeouts even if they're otherwise ignored. \$\endgroup\$
    – Flambino
    Jan 16, 2014 at 20:38
  • \$\begingroup\$ Oh, you're right. +1 then :) \$\endgroup\$ Jan 17, 2014 at 7:45
2
\$\begingroup\$

Less readable, but another way.

var prevNext = (e.keyCode == 38) ? "prev" : 
                   (e.keyCode == 40) : "next" : null;
if ( prevNext && !$html.hasClass('client-loading') && canAnim !== false ) {
    $('.project-current')[prevNext]('.project').click();   
    canAnim = false;
    setTimeout(function() { canAnim = true;},2000); }
}
\$\endgroup\$
1
  • \$\begingroup\$ think on the 2nd line it should be e.keyCode == 40 ? \$\endgroup\$
    – Stuart
    Dec 21, 2012 at 18:40
0
\$\begingroup\$

This is about as concise as i think it will reasonably get. There might be a better way to handle double presses then a global variable timeout.

if ( e.keyCode == 38 || e.keycode == 40 ) 
{
    if( $html.hasClass('client-loading') || canAnim === false ) return false;

    if (e.keyCode == 38) var $prevProj = $('.project-current').prev('.project').click();
    if (e.keyCode == 40) var $nextProj = $('.project-current').next('.project').click(); 
    canAnim = false;
    setTimeout(function() { canAnim = true;},2000); }

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

Another variation. I've taken the liberty of altering the method of timing, but that can easily be reverted.

$(document).keydown(function(e) {
    var now = (new Date()).getTime(), direction = ({38: 'prev', 40: 'next'})[e.keyCode];
    if (!direction || $html.hasClass('client-loading') || now < lastPressed + 2000) {
        return false;
    }
    lastPressed = now;
    $('.project-current')[direction]('.project').click();
}​
\$\endgroup\$
0
\$\begingroup\$
$(document).keydown(function (e) {

    //there is a very minor overhead when accessing properties
    //or querying elements in jQuery
    //cache the value in a variable if they are to be used repeatedly
    //
    //if they don't change during the course of the page's life,
    //or by dynamic means, cache them outside the handler
    //
    //I'll just assume .project-current is dynamic and will need requerying
    //everytime we call the handler

    var keycode = e.keyCode,
        loading = $html.hasClass('client-loading'),
        currentProject = $('.project-current');

    //run the code only on 38 and 40, when not loading and can animate
    //otherwise do nothing
    //why would you run code when it shouldn't?

    //when testing for boolean, it makes no sense comparing to true and false
    //you can use the variable directly to represent it's state

    if((keycode===38 || keycode===40) && !loading && canAnim ){

        //there is a minor optimization done when using "else"
        //when the first condition is met, conditions chained with else
        //will not be evaluated anymore. This means process time savings
        //without it, the second condition will still be evaluated

        if(keycode===38){
            currentProject.prev('.project').click();
        } else if(keyCode===40){
            currentProject.next('.project').click();
        }

        canAnim = false;
        setTimeout(function(){
            canAnim = true;
        },2000);

    }        
});
\$\endgroup\$
2
  • 1
    \$\begingroup\$ The line } else if (e.keyCode===40){ should presumably read keyCode instead of e.keyCode and anyway you could just have a simple } else { there since you have already checked keyCode is either 38 or 40. \$\endgroup\$
    – Stuart
    Dec 21, 2012 at 18:42
  • \$\begingroup\$ @Stuart thanks! kinda forgot that I already cached the keycode value. However, I added the additional "if 40" for clarity. If I omitted it, a future developer might miss reading the "if 38 || 40" at the top and might think that the "if 40" part was a "default" like in a switch statement. \$\endgroup\$
    – Joseph
    Dec 22, 2012 at 1:48
-4
\$\begingroup\$

The switch statement is made exactly for this kind of case.

$(document).keydown(function(e){
    var $proj;
    switch (e.keyCode) {
        case 38:
            if($html.hasClass('client-loading') || canAnim === false) {
                return false;
            }
            $proj = $('.project-current').prev('.project');
            break;

        case 40:
            if($html.hasClass('client-loading') || canAnim === false) {
                return false;
            }
            $proj = $('.project-current').next('.project');
            break;

        // Prevent rapid clicking
        case 38:
        case 40:
            canAnim = false;

            setTimeout(function(){
                canAnim = true;
            },2000);
            break;
    }

    $proj.click();
});

You can also notice the small refactoring I applied. Keeps some common stuff out of the switch, so that less code is in each case.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Won't work. Once you break, the rest of the cases are skipped. I.e. the timeout code will never be executed. \$\endgroup\$
    – Flambino
    Nov 19, 2012 at 18:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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