Simplifying a repetitive event handler

Question

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",
 success: function (admin) {
 if (admin == true) {
 window.location = urllink;
 }
 }
 });
 };
 });
 }
 else {
 $(document).keyup(function (e) {
 if (e.which == 112) {
 $.ajax({
 type: "POST",
 url: "/submit/role/",
 data: {},
 dataType: "json",
 success: function (admin) {
 if (admin == true) {
 window.location = urllink;
 }
 }
 });
 };
 });
 };

Answer 1

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) {
 if (admin == true) {
 window.location = urllink;
 }
 }
 });
 };
});

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!

• pllee: 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
• Anonymous: keyCode should have a name that reflects what the key does — e.g. something like saveKey, deleteKey, etc.
• David Harkness: @pilee What value of admin will cause those two if tests to act differently? Note that it's using == and not ===.
• Anonymous: 2 == true ⇒ false.
• Malachi: it would be better with a parameter, what if they want to change the codes, or add codes?

Answer 2

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",
 success: function (admin) {
 if (admin == true) {
 window.location = urllink;
 }
 }
 });
 };
});

Your function would look like this:

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

And then your code turns into this:

 if (isNodeWebkit) {
 $installKeyHandler(113);
 } else {
 $installKeyHandler(112);
 }

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, clean and decouples the operation from the input.

• David Harkness: Don't forget about urllink. Without the surrounding context, I was hesitant to alter the code too much.
• Malachi: 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.
• Anonymous: runMyCode should have a meaningful name, like installKeyHandler.
• David Harkness: If you're going to put the function inline into that same code, I see no need for a function myself.
• Malachi: @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.

Answer 3

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",
 success: function (admin) {
 if (admin == true) {
 window.location = urllink;
 }
 }
 });
 };
 });

• Malachi: this is a good idea, but you still have Magic Numbers.