3
\$\begingroup\$

Since I'm still learning jQuery, I would like to have some feedback on my code:

var showImgage = function()
{
	var selection = $(this).val();
	
	switch(selection) 
	{
		case 'Monica Bellud':
		$('#Belluci').removeClass().addClass('show');
		$('#Andres').removeClass().addClass('hide');
		$('#Seydoux').removeClass().addClass('hide');
		break
		
		case 'Lea Seydoux':
		$('#Seydoux').removeClass().addClass('show');
		$('#Belluci').removeClass().addClass('hide');
		$('#Andres').removeClass().addClass('hide');
		break
		
		case 'Ursula Andress':
		$('#Andres').removeClass().addClass('show');
		$('#Belluci').removeClass().addClass('hide');
		$('#Seydoux').removeClass().addClass('hide');
		break
		
		default:
		selection.removeClass();
	}
}
<div>
 <label for = "bondgirl">Wie van de onderstaande dames is géén Bond Girl in de nieuwe film 'SPECTRE'?</label>
 <input type="radio" value="Monica Bellud" name="girlchoice" id="choicemonica" checked /><label for="choicemonica" class="sidelabel">Monica Bellud</label>
 <input type="radio" value="Lea Seydoux" name="girlchoice" id="choicelea" /><label for="choicelea" class="sidelabel">Lea Seydoux</label>
 <input type="radio" value="Ursula Andress" name="girlchoice" id="choiceursula" /><label for="choiceursula" class="sidelabel">Ursula Andress</label>
</div>

I simply want to change the image (which is in my div of my HTML) for each radiobutton. It works, but it looks like I'm duplicating my code 3 times. How can I make this easier?

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Dec 30, 2014 at 13:26
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

I would probably add a class/something to use as a selector (in this case I added girls) to make lookup easier. You can do this in other ways such as getting the values of the actors map so do it as you will.

Also your default case is calling removeClass on a string as selection is the current contents of whatever the event is hooked to.

// Actor name -> the corresponding element selector on the page
var actorsIdMap = {
 'Monica Bellud': '#Bellud',
 'Lea Seydoux': '#Seydoux',
 'Ursula Andress': '#Andres' 
}
var showImage = function() {
 var selection = $(this).val();
 if (selection in actorsIdMap) { 
 // Hide the other actors
 $('.girls').removeClass('hide show');
 $(actorsIdMap[selection]).addClass('show');
 } else {
 // Default case what do?
 }
}
answered Dec 30, 2014 at 18:34
\$\endgroup\$
1
\$\begingroup\$

The first thing that you can do will reduce the code written but slightly increase the number of images that you hide. Before the switch, hide all the images, like so:

 $('#Belluci').removeClass().addClass('hide');
 $('#Seydoux').removeClass().addClass('hide');
 $('#Andres').removeClass().addClass('hide');

Or

 $('input[name=girlchoice].show').removeClass('show').addClass('hide');

Consider changing the value of the radio buttons like so:

<input type="radio" value="Belluci" name="girlchoice" id="choicemonica" checked /><label for="choicemonica" class="sidelabel">Monica Bellud</label>
<input type="radio" value="Seydoux" name="girlchoice" id="choicelea" /><label for="choicelea" class="sidelabel">Lea Seydoux</label>
<input type="radio" value="Andres" name="girlchoice" id="choiceursula" /><label for="choiceursula" class="sidelabel">Ursula Andress</label>

Then you can just say

$('#' . selection).removeClass().addClass('show');

So the whole function would be

var showImgage = function()
{
 var selection = $(this).val();
 $('input[name=girlchoice].show').removeClass('show').addClass('hide');
 $('#' . selection).removeClass('hide').addClass('show');
}

You lose the default case, but I'm not sure that you really needed it.

Note: it might be

 $('input.show[name=girlchoice]').removeClass('show').addClass('hide');

instead. I'm not set up for testing at the moment.

answered Dec 30, 2014 at 18:53
\$\endgroup\$
1
  • \$\begingroup\$ Doesn't work for me :o \$\endgroup\$ Commented Jan 16, 2015 at 14:51

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.