Whats wrong with this code?

Question

function menuhover(id,state){
    if(id=="home"){
     if(state=="over"){
      document.getElementById(id).src="pages/styles/images/home1hover.png";
     } else if(state=="out"){
      document.getElementById(id).src="pages/styles/images/home1.png";
     }
    } else if(id=="news"){
     if(state=="over"){
      document.getElementById(id).src="pages/styles/images/news2hover.png";
     } else if(state=="out"){
      document.getElementById(id).src="pages/styles/images/news2.png";
     }
    } else if(id=="register"){
     if(state=="over"){
      document.getElementById(id).src="pages/styles/images/register3hover.png";
     } else if(state=="out"){
      document.getElementById(id).src="pages/styles/images/register3.png";
     }
    } else if(id=="contrib"){
     if(state=="over"){
      document.getElementById(id).src="pages/styles/images/contributor4hover.png";
     } else if(state=="out"){
      document.getElementById(id).src="pages/styles/images/contributor4.png";
     }
    } else if(id=="login"){
     if(state=="over"){
      document.getElementById(id).src="pages/styles/images/login5hover.png";
     } else if(state=="out"){
      document.getElementById(id).src="pages/styles/images/login5.png";
     }
    }
}

it is referenced in

<div class="menu">
    <img class="banner" src="pages/styles/images/banner.png" border="0"/>
    <p class="link"><a href="http://www.realmsofruin.co.cc"&gt;&lt;img id="home" src="pages/styles/images/home1.png" border="0" onmouseover="menuhover(home,over)" onmouseout="menuhover(home,out)"/></a></p>
    <p class="link"><a href=""><img id="news" src="pages/styles/images/news2.png" border="0" onmouseover="menuhover(news,over)" onmouseout="menuhover(news,out)"/></a></p>
    <p class="link"><a href=""><img id="register" src="pages/styles/images/register3.png" border="0" onmouseover="menuhover(register,over)" onmouseout="menuhover(register,out)"/></a></p>
    <p class="link"><a link=""><img id="contrib" src="pages/styles/images/contributor4.png" border="0" onmouseover="menuhover(contrib,over)" onmouseout="menuhover(contrib,out)"/></a></p>
    <p class="link"><a link=""><img id="login" src="pages/styles/images/login5.png" border="0" onmouseover="menuhover(login,over)" onmouseout="menuhover(login,out)"/></a></p>
</div>

and the error console tells me this

over is not defined out is not defined..

how am i supposed to define them??

Answer 1

You just need to put quotes around your variables that you are passing:

onmouseover="menuhover('home', 'over')"

without them, it's looking for a variable called home or over which doesn't exist.

Answer 2

Should it be something like

onmouseover="menuhover('login','over')"

rather than

onmouseover="menuhover(login,over)"

ie the parameters to the function calls need to be literal strings...

Answer 3

As nickf correctly pointed out, you need to enclose the strings you are passing into you function in quotes.

You can also short-cut the manual strings by passing in the ID of the image:

onmouseover="menuhover(this.id, 'over')"

You can take this a step further and use the Event object that is generated, and thus remove the need to hard-code the 'over' and 'out' parts. (Oh and take a look at the Switch statement to get rid of that nasty mess of if {} elseif {} else {} blocks)

And for a third option, if all you are doing is setting hover/out images, you can do all of this with CSS.

Answer 4

In addition to the correct diagnosis by @nickf, I'd like to suggest that you simplify the code somewhat.

function menuhover(imgElement){
    imgElement.src = "pages/styles/images/" + imgElement.id + "hover.png";
}

function menuout(imgElement){
    imgElement.src = "pages/styles/images/" + imgElement.id + ".png";
}


<div class="menu">
    <img class="banner" src="pages/styles/images/banner.png" border="0"/>
    <p class="link"><a href="http://www.realmsofruin.co.cc"&gt;&lt;img id="home" src="pages/styles/images/home.png" border="0" onmouseover="menuhover(this)" onmouseout="menuout(this)"/></a></p>
    <p class="link"><a href=""><img id="news" src="pages/styles/images/news.png" border="0" onmouseover="menuhover(this)" onmouseout="menuout(this)"/></a></p>
    <p class="link"><a href=""><img id="register" src="pages/styles/images/register.png" border="0" onmouseover="menuhover(this)" onmouseout="menuout(this)"/></a></p>
    <p class="link"><a link=""><img id="contrib" src="pages/styles/images/contrib.png" border="0" onmouseover="menuhover(this)" onmouseout="menuout(this)"/></a></p>
    <p class="link"><a link=""><img id="login" src="pages/styles/images/login.png" border="0" onmouseover="menuhover(this)" onmouseout="menuout(this)"/></a></p>
</div>

In fact, if you were to adopt the use of a library such as jQuery, you could simplify the code even further.

Answer 5

As iAn suggests, the smart use of CSS could eliminate a whole load of javascript - I was so focussed on improving your implementation, I didn't spot the obvious alternative) Assuming that your images are 16 by 16, I'd do something like this...

.link a { //defines the size of all <a> elements that appear insie an element with the 'link' class
  width: 16px;
  height 16px;
  background-repeat: norepeat;
  background-position: 0px, 0px;
}

a.home{ //defines the appearance of the 'home' link, when the cursor is NOT over it
    background-image: url("pages/styles/images/home.png");
}

a.home:hover{ //defines the appearance of the 'home' link, when the cursor is hovering over it
    background-image: url("pages/styles/images/homehover.png");
}

a.news{
    background-image: url("pages/styles/images/news.png");
}

a.news:hover{
    background-image: url("pages/styles/images/newshover.png");
}

a.register{
    background-image: url("pages/styles/images/register.png");
}

a.register:hover{
    background-image: url("pages/styles/images/registerhover.png");
}

a.contrib{
    background-image: url("pages/styles/images/contrib.png");
}

a.contrib:hover{
    background-image: url("pages/styles/images/contribhover.png");
}


<div class="menu">
<img class="banner" src="pages/styles/images/banner.png" border="0"/>
<p class="link"><a href="http://www.realmsofruin.co.cc"&gt;&amp;#160;&lt;/a&gt;&lt;/p&gt;
<p class="link"><a href="" class="home">&#160;</a></p>
<p class="link"><a href="" class="news">&#160;</a></p>
<p class="link"><a link="" class="register">&#160;</a></p>
<p class="link"><a link="" class="contrib">&#160;</a></p>
</div>

No javascript required, and probably compatible with most browsers - although I'd be tempted to put a title attrubute on each element, to help with screen readers and accessibility issues.