# Simplifying a repetitive event handler

Is there a more efficient way of writing this? It seems like so much redundancy that this can be greatly reduced. Basically the only difference is if isNodeWebkit is true then run a function using key code 113 or 112

    if (isNodeWebkit) {
$(document).keyup(function (e) { if (e.which == 113) {$.ajax({
type: "POST",
url: "/submit/role/",
data: {},
dataType: "json",
}
}
});
};
});
}
else {
$(document).keyup(function (e) { if (e.which == 112) {$.ajax({
type: "POST",
url: "/submit/role/",
data: {},
dataType: "json",
}
}
});
};
});
};

-

Yes! Introduce a keyCode variable to hold the one difference between the two otherwise identical code blocks.

var keyCode = isNodeWebkit ? 113 : 112;
$(document).keyup(function (e) { if (e.which == keyCode) {$.ajax({
type: "POST",
url: "/submit/role/",
data: {},
dataType: "json",
success: function (admin == true) {
}
}
});
};
});


Edit: Reverted equality tests since I don't know what you expect to appear in admin, and heck, perhaps some browsers convert the key code to a string!

-
To be clear for beginners if (admin == true) is not identical to if (admin) . For this instance that statement is probably correct but it all depends on the type of admin –  pllee May 19 '14 at 21:58
keyCode should have a name that reflects what the key does — e.g. something like saveKey, deleteKey, etc. –  Anonymous May 19 '14 at 22:20
@pilee What value of admin will cause those two if tests to act differently? Note that it's using == and not ===. –  David Harkness May 19 '14 at 22:23
2 == true ⇒ false. –  Anonymous May 19 '14 at 22:40
it would be better with a parameter, what if they want to change the codes, or add codes? –  Malachi May 20 '14 at 18:09

what you should really do here is turn this part into a function with a parameter for your keyCode

    $(document).keyup(function (e) { if (e.which == 113) {$.ajax({
type: "POST",
url: "/submit/role/",
data: {},
dataType: "json",
}
}
});
};
});


var $installKeyHandler = function(keyCode) {$(document).keyup(function (e) {
if (e.which == keyCode) {
$.ajax({ type: "POST", url: "/submit/role/", data: {}, dataType: "json", success: function (admin) { if (admin) { window.location = urllink; } } }); }; }); }  then your code turns into this  if (isNodeWebkit) {$installKeyHandler(113);
} else {
$installKeyHandler(112); }  and because I like Ternary (I think it looks cool) I would make this a little shorter $installKeyHandler(isNodeWebKit ? 113 : 112)


you are using a Magic Number to dictate what happens in code that is exactly the same in the if block and the else block.

By creating the function and then calling it with the Ternary you are writing far less code, and it is nice and clean.

-
Don't forget about urllink. Without the surrounding context, I was hesitant to alter the code too much. –  David Harkness May 19 '14 at 22:25
the function will be run in the same spot as the code was before, so I think that the variable (whatever it is) will still hold the correct data when that function is run, especially if the function is created right before it is used. if it is needed somewhere else then that will be a different story though. –  Malachi May 19 '14 at 22:31
runMyCode should have a meaningful name, like installKeyHandler. –  Anonymous May 19 '14 at 22:43
If you're going to put the function inline into that same code, I see no need for a function myself. –  David Harkness May 19 '14 at 23:15
@DavidHarkness it shortens the and lessens the chance of typos because there is less code to maintain. it's JavaScript, it was meant to be compact. –  Malachi May 20 '14 at 0:58

You can combine if and else condition and write it for single ajax call (as ajax call is same for both if and else part)

$(document).keyup(function (e) { // here if and else conditions written in OR condition if ((isNodeWebkit && e.which == 113) || (!isNodeWebkit && e.which == 112)) {$.ajax({
type: "POST",
url: "/submit/role/",
data: {},
dataType: "json",