15
\$\begingroup\$

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;
                        }
                    }
                });
            };
        });
    };
Improve this question
\$\endgroup\$

3 Answers 3

Reset to default
9
\$\begingroup\$

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!

Improve this answer
\$\endgroup\$
6
  • \$\begingroup\$ 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 \$\endgroup\$
    – pllee
    May 19, 2014 at 21:58
  • 2
    \$\begingroup\$ keyCode should have a name that reflects what the key does — e.g. something like saveKey, deleteKey, etc. \$\endgroup\$
    – Anonymous
    May 19, 2014 at 22:20
  • \$\begingroup\$ @pilee What value of admin will cause those two if tests to act differently? Note that it's using == and not ===. \$\endgroup\$
    – David Harkness
    May 19, 2014 at 22:23
  • \$\begingroup\$ 2 == true ⇒ false. \$\endgroup\$
    – Anonymous
    May 19, 2014 at 22:40
  • 1
    \$\begingroup\$ it would be better with a parameter, what if they want to change the codes, or add codes? \$\endgroup\$
    – Malachi ♦
    May 20, 2014 at 18:09
6
\$\begingroup\$

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.

Improve this answer
\$\endgroup\$
6
  • 1
    \$\begingroup\$ Don't forget about urllink. Without the surrounding context, I was hesitant to alter the code too much. \$\endgroup\$
    – David Harkness
    May 19, 2014 at 22:25
  • \$\begingroup\$ 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. \$\endgroup\$
    – Malachi ♦
    May 19, 2014 at 22:31
  • 2
    \$\begingroup\$ runMyCode should have a meaningful name, like installKeyHandler. \$\endgroup\$
    – Anonymous
    May 19, 2014 at 22:43
  • \$\begingroup\$ If you're going to put the function inline into that same code, I see no need for a function myself. \$\endgroup\$
    – David Harkness
    May 19, 2014 at 23:15
  • \$\begingroup\$ @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. \$\endgroup\$
    – Malachi ♦
    May 20, 2014 at 0:58
2
\$\begingroup\$

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;
                    }
                }
            });
        };
    });
Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ this is a good idea, but you still have Magic Numbers. \$\endgroup\$
    – Malachi ♦
    May 20, 2014 at 13:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy