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 3 down vote favorite

I have some experimental script, that makes an animation of a ul list element background-position, when the list element is hovered. Is there another way to manage this task? Or just optimize this code?

I want to make a menu that has elements with an animated background and have an idea about rotoscope style design or something in that direction. I'm thinking in cross-browser compatibility way too (: In this stage this code runs fine in Opera, Internet Explorer 6, Chrome, Firefox 2.0, and Firefox 4.0.

$(document).ready(function(){
  $("li.animate").hover(
    function () {

      var param = '0 0';
      ids = setInterval(function() {
        if ( c >= 5 ) {
          c = 1;
          $('.animate').css('backgroundPosition', '0 0');
        }
        else {
          param = (-100 * c++ )+'px 0';
          //alert(param);
          $('.animate').css('backgroundPosition', param);
          if ( c > 4 ) $('.animate').css('backgroundPosition', '0 0');
        }
      }, 40);
    },
    function () {
      $('.animate').css('backgroundPosition', '0 0');
      clearInterval(ids);
    }
  );
});

http://jsfiddle.net/jurisKaste/nDgSE/

share|improve this question
2  
since you already use a gif, you could use a animated gif on the hover. it would save you all this code and have the same visual effect. –  meo Jun 27 '11 at 23:47
    
yes, people already told me about this (gif animation), but I want to make navigation bar that uses css sprites... maybe it sounds awkward - but i need to control this animation fbf, it's just my idea :D –  wolf3d Jun 28 '11 at 7:49
    
i think its the total overkill to do this with JS... But i am willing to give you a feedback on your code –  meo Jun 28 '11 at 11:22

2 Answers 2

up vote 2 down vote accepted

First of all i mention again, that i would use a GIF animation to solve this and not a JS. Its a very suboptimal solution to a simple problem.

Your code in the code block is missing two var declarations you have in you jsFiddle example.

Here are the things i would change:

In the most cases, when you have to type the same value twice you do something wrong. Use variables to store your stuff. Makes code more maintainable, easy to read, faster in some cases etc... And give your variables names that speak for them selves. Since you don't write any comments, its even more important.

Cache your jQuery objects in variables. By selecting them again and again, your code gets slow and is hard to mainain. Here an example:

$animate = $("li.animate");
$animate.hover(/* blabalbal */);

Put all your code in a function and pass variable stuff (things that could change) as parameters. Always ask your self what could change if you want to reuse the same code for a different CSS / HTML configuration. (Like different size of the element)

var yourFunction = function( $elem ){
  $elem.each(function(){
   //do something
  });
};

$(function(){ //document ready
  yourFunction( $("whatever element you want to handle") );
});

Then you can call your function in the document ready and you don't have to write all your stuff into your document ready function.

In general split your code in as small generic functions as possible. (Morden browser compile functions that are used often, can make a huge performance differences)

Inside your hover functions you reselect $('.animate'). What are you doing if you have more then one element that has this class? You can refer to the element you are hovering with this inside your hover function:

   $(something).hover(function(){ $(this) //give you back a jQuery selection of the hovered element 
   },function(){});

The biggest problem i see in your code is that you have declared your variables very globally, what are you doing if you need to apply this code twice on the same page?

Hope this helps.

share|improve this answer
up vote 2 down vote

  • You left out the declarations of the variables ids and c in the posted code. Also you shouldn't unnecessarily let them be global variables. You can move ids into the document ready function and c into the first hover function.

  • You are very inconsistent:

    • You first declare c to be 0, but in the animation you reset it to 1 instead. Choose one.

    • When c reaches 5 you first set the background position to -500px 0 and then in the next line (if ( c > 4 ) ...) immediately back to 0 0, which happens in the next call of the interval function again. As I see it, the line if ( c > 4 ) ... is completely unnecessary.

share|improve this answer
    
about inconsistent 'c' variable... i did it cause im using postfix increment... but yes i read the code another time and Your arguments are true! sometimes i just write code and if it works i leave it as is... only this time i thought about optimization and logic question and posted here, thx for replay –  wolf3d Jun 28 '11 at 18:09

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.