1
\$\begingroup\$

I made this Javascript to hide, show and change some DIVs but I believe it's not really good code. Can you help me to make it better?

function colorM(n) {
 switch(n){
 case 1:
 document.getElementById("slideM1").style.backgroundColor="#009CFF";
 document.getElementById("slideM1").className="show";
 document.getElementById("slideM2").style.backgroundColor="silver";
 document.getElementById("slideM3").style.backgroundColor="silver";
 document.getElementById("slideM4").style.backgroundColor="silver";
 document.getElementById("slideM5").style.backgroundColor="silver";
 document.getElementById("slideM2").className="";
 document.getElementById("slideM3").className="";
 document.getElementById("slideM4").className="";
 document.getElementById("slideM5").className="";
 document.getElementById("slideC1").style.display="block";
 document.getElementById("slideC2").style.display="none";
 document.getElementById("slideC3").style.display="none";
 document.getElementById("slideC4").style.display="none";
 document.getElementById("slideC5").style.display="none";
 break;
 case 2:
 document.getElementById("slideM2").style.backgroundColor="#009CFF";
 document.getElementById("slideM2").className="show";
 document.getElementById("slideM1").style.backgroundColor="silver";
 document.getElementById("slideM3").style.backgroundColor="silver";
 document.getElementById("slideM4").style.backgroundColor="silver";
 document.getElementById("slideM5").style.backgroundColor="silver";
 document.getElementById("slideM1").className="";
 document.getElementById("slideM3").className="";
 document.getElementById("slideM4").className="";
 document.getElementById("slideM5").className="";
 document.getElementById("slideC1").style.display="none";
 document.getElementById("slideC2").style.display="block";
 document.getElementById("slideC3").style.display="none";
 document.getElementById("slideC4").style.display="none";
 document.getElementById("slideC5").style.display="none";
 break;
 case 3:
 document.getElementById("slideM3").style.backgroundColor="#009CFF";
 document.getElementById("slideM3").className="show";
 document.getElementById("slideM1").style.backgroundColor="silver";
 document.getElementById("slideM2").style.backgroundColor="silver";
 document.getElementById("slideM4").style.backgroundColor="silver";
 document.getElementById("slideM5").style.backgroundColor="silver";
 document.getElementById("slideM1").className="";
 document.getElementById("slideM2").className="";
 document.getElementById("slideM4").className="";
 document.getElementById("slideM5").className="";
 document.getElementById("slideC1").style.display="none";
 document.getElementById("slideC2").style.display="none";
 document.getElementById("slideC3").style.display="block";
 document.getElementById("slideC4").style.display="none";
 document.getElementById("slideC5").style.display="none";
 break;
 case 4:
 document.getElementById("slideM4").style.backgroundColor="#009CFF";
 document.getElementById("slideM4").className="show";
 document.getElementById("slideM1").style.backgroundColor="silver";
 document.getElementById("slideM2").style.backgroundColor="silver";
 document.getElementById("slideM3").style.backgroundColor="silver";
 document.getElementById("slideM5").style.backgroundColor="silver";
 document.getElementById("slideM1").className="";
 document.getElementById("slideM2").className="";
 document.getElementById("slideM3").className="";
 document.getElementById("slideM5").className="";
 document.getElementById("slideC1").style.display="none";
 document.getElementById("slideC2").style.display="none";
 document.getElementById("slideC3").style.display="none";
 document.getElementById("slideC4").style.display="block";
 document.getElementById("slideC5").style.display="none";
 break;
 case 5:
 document.getElementById("slideM5").style.backgroundColor="#009CFF";
 document.getElementById("slideM5").className="show";
 document.getElementById("slideM1").style.backgroundColor="silver";
 document.getElementById("slideM2").style.backgroundColor="silver";
 document.getElementById("slideM3").style.backgroundColor="silver";
 document.getElementById("slideM4").style.backgroundColor="silver";
 document.getElementById("slideM1").className="";
 document.getElementById("slideM2").className="";
 document.getElementById("slideM3").className="";
 document.getElementById("slideM4").className="";
 document.getElementById("slideC1").style.display="none";
 document.getElementById("slideC2").style.display="none";
 document.getElementById("slideC3").style.display="none";
 document.getElementById("slideC4").style.display="none";
 document.getElementById("slideC5").style.display="block";
 break;
 default: 
 document.getElementById("slideM1").style.backgroundColor="#009CFF";
 document.getElementById("slideM1").className="show";
 document.getElementById("slideM2").style.backgroundColor="silver";
 document.getElementById("slideM3").style.backgroundColor="silver";
 document.getElementById("slideM4").style.backgroundColor="silver";
 document.getElementById("slideM5").style.backgroundColor="silver";
 document.getElementById("slideM2").className="";
 document.getElementById("slideM3").className="";
 document.getElementById("slideM4").className="";
 document.getElementById("slideM5").className="";
 document.getElementById("slideC1").style.display="block";
 document.getElementById("slideC2").style.display="none";
 document.getElementById("slideC3").style.display="none";
 document.getElementById("slideC4").style.display="none";
 document.getElementById("slideC5").style.display="none";
 break;
 }
}
Quentin Pradet
7,0641 gold badge25 silver badges44 bronze badges
asked May 23, 2013 at 8:56
\$\endgroup\$
1
  • 1
    \$\begingroup\$ For starters: Instead of clearing the className of every div that you want to hide, assign a class name that already has the background-color: 'silver' and display: none. That alone saves you a lot of lines. \$\endgroup\$ Commented May 23, 2013 at 9:09

1 Answer 1

3
\$\begingroup\$

Loop through the ID's instead of doing a switch. This should do the same as yours

function colorM(id){
 // if not a number
 if (!(!isNaN(parseFloat(n)) && isFinite(n))) id =1;
 // default for numbers
 if (id > 5 || id < 1) id = 1;
 for (var i = 1; i <= 5; i++){
 if (id == i){
 document.getElementById("slideM" + i).style.backgroundColor="#009CFF";
 document.getElementById("slideM" + i).className="show";
 document.getElementById("slideC" + i).style.display="block";
 }
 else {
 document.getElementById("slideM" + i).style.backgroundColor="silver";
 document.getElementById("slideM" + i).className="";
 document.getElementById("slideC" + i).style.display="none";
 }
 }
}

Edit: added a check if id is numeric AND id is within range

answered May 23, 2013 at 10:29
\$\endgroup\$
3
  • \$\begingroup\$ That's almost perfect but I need also default state. Which is explained in default case. \$\endgroup\$ Commented May 23, 2013 at 11:47
  • \$\begingroup\$ The default is the same as 1? So make it in such a way that if you are trying to default it, (it is not any of the 5) it'll give 1 to the function \$\endgroup\$ Commented May 23, 2013 at 13:10
  • \$\begingroup\$ added a check in it! \$\endgroup\$ Commented May 23, 2013 at 13:13

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.