Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
                        }
                    }
                });
            };
        });
    };
share|improve this question

3 Answers 3

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) {
                if (admin) {
                    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!

share|improve this answer
    
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 at 21:58
2  
keyCode should have a name that reflects what the key does — e.g. something like saveKey, deleteKey, etc. –  Anonymous May 19 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 at 22:23
    
2 == true ⇒ false. –  Anonymous May 19 at 22:40
1  
it would be better with a parameter, what if they want to change the codes, or add codes? –  Malachi May 20 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",
                success: function (admin) {
                    if (admin == true) {
                        window.location = urllink;
                    }
                }
            });
        };
    });

Your function is 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;
                    }
                }
            });
        };
    });
}

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.

share|improve this answer
1  
Don't forget about urllink. Without the surrounding context, I was hesitant to alter the code too much. –  David Harkness May 19 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 at 22:31
2  
runMyCode should have a meaningful name, like installKeyHandler. –  Anonymous May 19 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 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 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",
                success: function (admin) {
                    if (admin == true) {
                        window.location = urllink;
                    }
                }
            });
        };
    });
share|improve this answer
    
this is a good idea, but you still have Magic Numbers. –  Malachi May 20 at 13:31

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.