3
\$\begingroup\$

I am doing a background image animation using the function. It works, but I think I am doing more coding than what it requires. Can anyone please help me to minimize it?

Don't recommend the CSS animations, I am working for IE9.

Live

var call = function () {

var x = 0;

    return function () {
        x++;
        x = x > 4 ? x=1 : x;
    $('#small').prop('class','').addClass('s'+x)
    .text(x)
    .animate({opacity: 0}, 3000, function() {
        call();
    });

        var y = x+1;

        $('#big').prop('class','').addClass('s'+y);

        $('#small').css({
            opacity:1
        })

    }

}();

call();
#big {
    width: 100px;
    height: 100px;
    position:relative;
    color:#fff;
  }

#small {
    position:absolute;
    left:0;
    top:0;
    width:100%;
    height:100%;
    color:#fff;
}


.s1,
.s5{
    opacity:1;
    background:gray;
}

.s2 {
    background:blue;
    opacity:1;
}

.s3 {
    background:yellow;
    opacity:1;
    color:#000;
}
.s4 {
    background:brown;
    opacity:1;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
<div id="big" class="s2">
    <div id="small" class="s1"></div>
</div>

\$\endgroup\$
3
\$\begingroup\$

The #big and #small divs are actually the same size. Therefore, I would rename them to #outer and #inner. You can use the same CSS rules for both.

There's no need to specify the opacity in the stylesheet, since the opacity will be controlled by the jQuery animation.

I'd renumber your classes starting from 0 so that you can take advantage of the modulo operator. The JavaScript code just looks cleaner with %.

The function-returned-by-a-function would be easier to understand if you make the outer scope into an object. It also deserves to have a better name than call.

All of the jQuery operations on $('#small') should be done in the same chain.

Clearing the class then calling addClass() is a roundabout way of just setting the class property.

function() { call(); } could just be simplified to call.

function ColorAnimation(speed) {
    var x = 0;
    var self = this;

    this.run = function() {
        x = (x + 1) % 4;
        var y = (x + 1) % 4;
        $('#outer').prop('class', 's' + y);
        $('#inner').prop('class', 's' + x)
                   .text(x + 1)
                   .css({ opacity: 1 })
                   .animate({opacity: 0}, speed, self.run);
    };
};

new ColorAnimation(3000).run();
#outer, #inner {
    width: 100px;
    height: 100px;
    position:relative;
    color:#fff;
}

.s0 {
    background:gray;
}
.s1 {
    background:blue;
}
.s2 {
    background:yellow;
    color:#000;
}
.s3 {
    background:brown;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
<div id="outer" class="s2">
    <div id="inner" class="s1"></div>
</div>

\$\endgroup\$
2
\$\begingroup\$

This seems to work rather well for what it does (why in the world IE9 support is so crucial, I don't know). There are small improvements, though:

var call = function (x) {
    /* You don't need to return a function, you could just pass the x value anyway.
     * Also, you are already setting x so there no need for x=1 in the first case.
     * Because you can also forget to pass x to this function, first check if x exists
     * and is smaller than 4, then increment, otherwise default to 1.
     * UPDATE: Sorry, my bad, check if x exists (otherwise set it to 1),
     * then check if its bigger than 4 (otherwise increment)
     */
    x = !x ? 1 : (x >= 4 ? 1 : x+1);
    // there no point in setting your opacity at the end, set it before
    $('#small').prop('class','').addClass('s'+x)
        .text(x)
        .css("opacity", 1)
        .animate({opacity: 0}, 3000, function() {
            call(x);
        });
    // since you only use y once, there no need to do the extra step of setting it
    $('#big').prop('class','').addClass('s'+ (x + 1));    
};

call();

var call = function (x) {
    x = !x ? 1 : (x >= 4 ? 1 : x+1);
        $('#small').prop('class','').addClass('s'+x)
        .text(x)
        .css("opacity", 1)
        .animate({opacity: 0}, 3000, function() {
            call(x);
        });
    $('#big').prop('class','').addClass('s'+ (x + 1));    
};

call();
#big {
    width: 100px;
    height: 100px;
    position:relative;
    color:#fff;
  }

#small {
    position:absolute;
    left:0;
    top:0;
    width:100%;
    height:100%;
    color:#fff;
}


.s1,
.s5{
    opacity:1;
    background:gray;
}

.s2 {
    background:blue;
    opacity:1;
}

.s3 {
    background:yellow;
    opacity:1;
    color:#000;
}
.s4 {
    background:brown;
    opacity:1;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
<div id="big" class="s2">
    <div id="small" class="s1"></div>
</div>

Update removed modulo, it complicates things a bit here. We actually need to check if x is bigger than 3 (or bigger and equal to 4) to make this work.

\$\endgroup\$
  • \$\begingroup\$ It seems, not working as per requirement. can you please update in to jsfiddle please. (i provided the link) \$\endgroup\$ – 3gwebtrain Jan 19 '15 at 9:59
  • \$\begingroup\$ I am finding issue here : x = !x || x > 4 ? 1 : x++; it's not increments! \$\endgroup\$ – 3gwebtrain Jan 19 '15 at 10:02
  • 1
    \$\begingroup\$ Sorry my logic got a bit bummed there. The solution is: x = !x ? 1 : (x > 4 ? 1 : x+1); (set x as (when there is no x: 1, ottherwise (when x is bigger than 4: 1, otherwise incremenet x by 1))) \$\endgroup\$ – somethinghere Jan 19 '15 at 10:17
  • 1
    \$\begingroup\$ your x logic is a bit of. An extra if is now done every time the function is called. Readability is also not there. Why not use modulo? x++; x = x % 4; or even combine them into one ++x % 4; You are also selecting the DOM element everytime the function is created. They should be cached \$\endgroup\$ – Pinoniq Jan 19 '15 at 10:20
  • \$\begingroup\$ @Pinoniq Thats quite a good idea, actually. I always forget about modulo. \$\endgroup\$ – somethinghere Jan 19 '15 at 10:21

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.