1
\$\begingroup\$

In the following piece of code I use the same trick three times. But cannot figure out how to write a common function that will do it for all of them. I know there is a toggle method in jQuery that does something similar but in this situation I wasn't able to get it do the job for me, so I wrote this:

// fallback for ie
if (navigator.appName == "Microsoft Internet Explorer") {
    var flag = true;
    $(".right-arrow").click(function () {
        if (flag) {
            $(".slider-frame").animate({ left: "-130px" }, 1000);
            flag = false;
        }
        else {
            $(".slider-frame").animate({ left: "236px" }, 1000);
            flag = true;
        }
        return false;
    });
}
    // more smooth CSS3-animation with move.js
else {
    var flag = true;
    $(".right-arrow").click(function () {
        if (flag) {
            move(".slider-frame").set("left", -130).duration("1s").end();
            flag = false;
        }
        else {
            move(".slider-frame").set("left", 236).duration("1s").end();
            flag = true;
        }
        return false;
    });
}

// eye that make passwords visible
var state = true;
$(".login-form form").append("<div class='eye'></div>");
$(".eye").click(function () {
    if (state) {
        $("#password").attr("type", "text");
        $(".eye").css("background-position", "0 -49px");
        state = false;
    }
    else {
        $("#password").attr("type", "password");
        $(".eye").css("background-position", "0 0");
        state = true;
    }
});
\$\endgroup\$
  • 2
    \$\begingroup\$ First off, before trying to DRY up your code, I'd suggest applying some more important concepts. For once, I'd suggest using proper feature detection instead of sniffing the appName. IE10 supports CSS3 animations, while old non-IE browsers do not. \$\endgroup\$ – Fabrício Matté Mar 3 '13 at 16:30
  • 1
    \$\begingroup\$ You need to place your code into some functions. That you can reuse your code. \$\endgroup\$ – Harrison Brock Mar 3 '13 at 16:48
2
\$\begingroup\$

Here's how to DRY it up:

var flag = true,
    isIE = navigator.appName == "Microsoft Internet Explorer";

$(".right-arrow").click(function () {
    if (isIE) {
        $(".slider-frame").animate({ left: flag ? "-130px" : "236px" }, 1000);
    }
    else {
        move(".slider-frame").set("left", flag ? -130 : 236).duration("1s");
    }    
    flag = ! flag;
    return false;
});

As pointed out by @FabrícioMatté, you should use feature detection instead of browser sniffing.


P.S. You should also learn to cache your selectors.

\$\endgroup\$
0
\$\begingroup\$

Here's an idea about some things you can do to make it leaner.

For each item that needs a click event, create an Immediately Executing Function. This will allow you to give local scope and not have a bazillion variables in the same scope to keep track of.

next, see what comonalities you can find for each IEF, and bring them to the top of the IEF.

I never like to attach event handlers via if/then statements. I'd rather figure out what I need to attach via if/then/whatever and only then attach.

Here's a start for ".slider-frame".

(function(){
    var selector = ".slider-frame",
        clickmethod, 
        flag = true, 
        measures = [236, -130]; 
    //this could be handled better, but whatever, fixing this wasn't the request
    if (navigator.appName == "Microsoft Internet Explorer") {
        clickmethod = function(){
            var value = measures[Number(flag)] + "px";
            $(selector).animate({ left: value }, 1000);
            flag = !flag;
        }
    }
    // more smooth CSS3-animation with move.js
    else {
        clickmethod = function(){
            var value = measures[Number(flag)];
            move(selector).set("left", value).duration("1s").end();
            flag = !flag;
        }
    }
    $(".right-arrow").click(clickmethod);
})();

Edit: Joseph Silber's point about caching "selectors" (or rather any reference to $(something)) is also a good point but if anything referred to by such an expression is replaced, your cached "selector" will no longer be valid. Furthermore, it's not clear to me if move(selector) can be cached or not, so I just left it be.

\$\endgroup\$
  • \$\begingroup\$ Thanks! It's interesting how you use the conversion of boolean to number. It's a good idea! \$\endgroup\$ – jstice4all Mar 3 '13 at 17:11

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.