# Keypress function conditional

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

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

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

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

-
$(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); } });  - 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.

-
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