current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

I have one container div which contains some children div members. From time to time, I need to run a custom blinking effects on some of the child div members. The effect steps are as follows:

  1. Change font color to Yellow
  2. Pause for 0.5 secs
  3. Change background color to yellow and text color to black
  4. Pause for 0.25 secs
  5. Change font color to Yellow & background to black
  6. Pause for 0.25 secs
  7. Change background color to yellow and text color to black
  8. Pause for 0.5 secs
  9. Repeat step 1 to 8

Below is the JavaScript code that I came up using jQuery to achieve the above effect. The code is working perfectly fine, but somehow I feel that this is not an elegant solution and there may be possible issue with how I pass $elements var from one function to another.

Is there anything wrong with this approach? How can this code be improved?

function startAnimation($container) {
    var $elements = $container.find(".animation");
    if ($elements.length) {           
        $elements.switchClass("mc-ylw-blk", "mc-blk-ylw", 0).show(0).delay(500).promise().done(function () {
            $elements.switchClass("mc-blk-ylw", "mc-ylw-blk", 0).delay(250).promise().done(function () {
                $elements.switchClass("mc-ylw-blk", "mc-blk-ylw", 0).delay(250).promise().done(function () {
                    $elements.switchClass("mc-blk-ylw", "mc-ylw-blk", 0).delay(500).promise().done(function () {                                                       
                            startAnimation($container);   //Repeat                   
                    });                 
                });
            });
        });
    }

}
share|improve this question
1  
Hi, and welcome to Code Review. Your question would benefit from having a JSFiddle to illustrate your task. Have you considered putting one together? –  rolfl May 22 '14 at 20:38
    
I already got the answer now. Next time I will definitely consider the jsfiddle. –  indusBull May 27 '14 at 21:16

3 Answers 3

up vote 2 down vote accepted

you can chain the delay and switchClass to $elements and take the $("#container").find(".animation"); outside the function like this 

function startAnimation($elements) {
$elements.switchClass("mc-ylw-blk", "mc-blk-ylw", 0)
      .delay(500).switchClass("mc-blk-ylw", "mc-ylw-blk", 0)
      .delay(250).switchClass("mc-ylw-blk", "mc-blk-ylw", 0)
      .delay(250).switchClass("mc-blk-ylw", "mc-ylw-blk", 0)
      .delay(500).promise().done(function () { 
          startAnimation($elements);
      });
}
var $elements=$("#container").find(".animation");
if ($elements.length) {
    startAnimation($elements);
}

http://jsfiddle.net/Y95rU/1/

share|improve this answer
    
Oh man.. thank you!! Now I'm wondering why I didn't think about this at first place. –  indusBull May 27 '14 at 21:15
up vote 1 down vote

Here is a version using setInterval() set at 250 at the second iteration and the sixth(in a set of 6) it skips thus taking twice the time, and then uses the modulus operator(dividing by 2 this time) to detirmine the proper class to display.

var $elements = $("#container").find(".animation");
var i = 0, x = 0;

if ($elements.length) {
    setInterval(function(){
        x++;
        if((x % 6) == 1 || (x % 6) == 5) return;
        class1 = ((i % 2) == 0) ? "mc-ylw-blk" : "mc-blk-ylw";
        class2 = ((i % 2) == 1) ? "mc-ylw-blk" : "mc-blk-ylw";
        $elements.switchClass(class1, class2, 0);
        i++;
    }, 250);
}

And here is the jsFiddle: http://jsfiddle.net/rWBa7/

share|improve this answer
1  
I fee that this code is more complicated and unreadable. Makes it harder to change it in future if requirement changes. –  indusBull May 27 '14 at 21:13
up vote 0 down vote

It would probably be more efficient to simply put all the logic into a loop rather than calling it recursively. You end up with a million copies of that function running until the page closes otherwise.

share|improve this answer

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.