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.

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

6 Answers 6

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);
});
share|improve this answer
    
I believe canAnim and setTimeout should be done before the second return early. –  Simon André Forsberg Jan 16 at 18:22
    
@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. –  Flambino Jan 16 at 20:38
    
Oh, you're right. +1 then :) –  Simon André Forsberg Jan 17 at 7:45

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); }
}
share|improve this answer
    
think on the 2nd line it should be e.keyCode == 40 ? –  Stuart Dec 21 '12 at 18:40

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); }

}
share|improve this answer

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();
}​
share|improve this answer
$(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);

    }        
});
share|improve this answer
1  
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. –  Stuart Dec 21 '12 at 18:42
    
@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. –  Joseph the Dreamer Dec 22 '12 at 1:48

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.

share|improve this answer
1  
Won't work. Once you break, the rest of the cases are skipped. I.e. the timeout code will never be executed. –  Flambino Nov 19 '12 at 18:29

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.