Tell me more ×
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?

Thanks!

$(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

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

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

Less readable, but another way.

var prevNext = (e.keyCode == 38) ? "prev" : 
                   (e.keyCode == 38) : "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

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
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.