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?

\$\endgroup\$
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?
    }
}
\$\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.

\$\endgroup\$
  • \$\begingroup\$ Doesn't work for me :o \$\endgroup\$ – Awa Jan 16 '15 at 14:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.