Toggling animations

Question

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

Answer 1

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.

Answer 2

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.