jQuery-animated navigation menu

Question

On the attached fiddle is what I hope will the be future UI for navigation on my own personal portfolio site. I was hoping you guys could critique the JavaScript I wrote to accomplish the most compatibility with the most browsers. I've tested as far back as IE 7 with this code and it works. Whereas the CSS solution is only IE 10+, and the jQuery Animate method is only usable in Chrome.

Below is the attached fiddle and all of the script I use to facilitate this process. Please be as brutal as possible. I am very new to JS and would like to know as much as possible!

http://jsfiddle.net/PhilFromHeck/um8Pp/

var key = "off";
var menuKey=["off","off","off","off"];
var menuID = [];
var menuState=[0,0,0,0];

$(document).ready(function () {
    init();
    $('.menu').mouseenter(function () {
      for (var i = 0; i < 4; i++) {
        if(menuID[i] == this){
          menuKey=["off","off","off","off"];
          menuKey[i] = "on";
        }
      }
      if(key=="off"){
        key="on";
        doMove();
      } 
    });
    $('.menu').mouseleave(function () {
          menuKey=["off","off","off","off"];
    });
});

function init() {
  menuID[0] = document.getElementById('menuOne'); 
  menuID[1] = document.getElementById('menuTwo'); 
  menuID[2] = document.getElementById('menuThree'); 
  menuID[3] = document.getElementById('menuFour'); 
}

function doMove() {
  var check = "no";
  if(key == "on"){
    for (var i = 0; i < 4; i++) {
      if(menuKey[i] == "on"){
        check = "yes";
        if(menuState[i] < 100){
          menuState[i] = menuState[i]+2;
          menuID[i].style.backgroundPosition="0px " + menuState[i] + "px";     
        }
      }
      if(menuKey[i] == "off"){
        if(menuState[i] > 0){
          check = "yes";
          menuState[i] = menuState[i]-2;
          menuID[i].style.backgroundPosition="0px " + menuState[i] + "px";     
        }
      }
    }
  if(check=="yes"){
    setTimeout(doMove,1); // call doMove in 20msec
  }
  else{
    key = "off";
  }
  }
}

Answer 1

For beginner code, that's reasonably good.

• Instead of calling setTimeout() yourself, why not let jQuery handle the animation for you with .animate()?

• Your indentation is wrong, as evidenced by

    }
    }
  }
  

  at the end. I believe that two spaces is too stingy for readability, and also encourages excessive nesting, which is poor programming practice. For example, the entire doMove() function should do nothing unless key == "on", so you could eliminate one layer of nesting by returning early:

  function doMove() {
      if (key != "on") {
          return;
      }
      ...
  }
  

• You use magic numbers 4 and 100. Don't hard-code 4; detect it as menuID.length instead. You should be able to detect the height as well; it not, then hard-code 100 as a named constant rather than being casually embedded as a loop limit.

• Storage of state feels clumsy. Use booleans (true and false) instead of strings ("on" and "off"). Naming could be better — I suggest menuElements rather than menuID, and animationPosition rather than menuState. You should be able to use fewer variables. For example, key and menuKey could be collapsed down to one scalar called activeMenuItem.

  var activeMenuItem;
  var menuElements = [];
  var animationPosition = [0, 0, 0, 0];
  
  $(document).ready(function () {
      init();
      $('.menu').mouseenter(function () {
          activeMenuItem = this;
          doMove();
      });
      $('.menu').mouseleave(function () {
          activeMenuItem = null;
      });
  });
  
  function doMove() {
      var check = false;
      for (var i = 0; i < menuElements.length; i++) {
          if (menuElements[i] == activeMenuItem) {
              if (animationPosition[i] < 100) {
                  animationPosition[i] += 2;
                  menuElements[i].style.backgroundPosition="0px " + animationPosition[i] + "px";
              }
              check = true;
          } else if (animationPosition[i] > 0) {
              animationPosition[i] -= 2;
              menuElements[i].style.backgroundPosition="0px " + animationPosition[i] + "px";
              check = true;
          }
      }
      if (check) {
          setTimeout(doMove, 1); // call doMove in 20msec
      }
  }
  

• Initialization of menuID scales poorly with additional menu items. Try selecting all $('.menu') instead of individually named elements.

Answer 2

In addition to my original answer, I'd like to point out three issues with your use of setTimeout().

First, your comment says that you wish to call doMove() again after 20 milliseconds, but your code actually sets the timer for 1 millisecond. Let's set it to 20, which produces a slow animation.

Next, you'll observe that if the pointer leaves a menu item and immediately moves over another, the animation can run at double speed. That is, while the original sequence of timers is still running, your mouseenter handler kicks off a second sequence of timers. You can prevent this by keeping track of whether a setTimeout() is still scheduled:

var mutex;

$(document).ready(function() {
    init();
    $('.menu').mouseenter(function() {
        activeMenuItem = this;
        if (!mutex) {
            doMove();
        }
    });
    ...
});

function doMove() {
    ...
    if (check) {
        mutex = setTimeout(doMove, 20);
    } else {
        mutex = null;
    }
}

Finally, there is an efficiency issue. As long as the pointer is hovering over a menu item, you will be frantically polling to see when to undo the animation. That would be bad for battery life on mobile devices. Instead, you want rest when nothing is moving, and trigger the reverse animation when the pointer leaves the active item.

    $(document).ready(function () {
        init();
        $('.menu').mouseenter(function () {
            activeMenuItem = this;
            if (!mutex) {
                doMove();
            }
        })
        .mouseleave(function () {     // You can chain jQuery calls
            activeMenuItem = null;
            if (!mutex) {             // Also trigger animation here
                doMove();
            }
        });
    });

    function doMove() {
        var animating = false;
        for (var i = 0; i < menuElements.length; i++) {
            if (menuElements[i] == activeMenuItem) {
                if (animationPosition[i] < 100) {
                    animationPosition[i] += 2;
                    menuElements[i].style.backgroundPosition="0px " + animationPosition[i] + "px";
                    animating = true; // Set animating=true only if something moved
                }
            } else if (animationPosition[i] > 0) {
                animationPosition[i] -= 2;
                menuElements[i].style.backgroundPosition="0px " + animationPosition[i] + "px";
                animating = true;
            }
        }
        if (animating) {
            mutex = setTimeout(doMove, 20); // call doMove in 20msec
        } else {
            mutex = null;
        }
    }

With that change, check is true only when an animation is in progress. As a side benefit, then, we can give it a clearer name: animating.