I think my code has too many functions... can it be written better?

# HTML

<div class="event_owner">
<p>
<label for="xxx">xxx</label>
</p>

<p>
<label for="yyy">yyy</label>
</p>
<input type="text" name="_event_owner[name]" autocomplete="off" value="">
</div>


# jQuery

$(".event_owner").click(function(){$(".event_owner input[type='text']").val($('.event_owner :radio:checked').attr("id"));$('.event_owner p').css({"opacity":"1"});
$('.event_owner input').not(":checked").parent("p").css({"opacity":"0.3"}); }); // when page loaded and input(radio) saved if($(".event_owner input[type='text']").length) {
$("input[id="+$(".event_owner input[type='text']").val()+"]").attr("checked","checked");
$('.event_owner input').not(":checked").parent("p").css({"opacity":"0.3"}); }  # Playground http://jsfiddle.net/Zf4UE/ - I'm wondering what you're trying to accomplish. You set the value of the text field with the radio buttons, but what if someone just writes something else in the field? Now the field no longer matches the selected radio button, and may be entirely invalid. So I'm not sure how it's supposed to work. – Flambino Nov 6 '12 at 3:13 ## 1 Answer There are a few points worth mentioning: 1. Your code does not actually function the way you want it to. $(".event_owner input[type='text']").length checks for the existence of that element, not whether its value is set.

2. Your radio buttons don't have a value attribute on them. They really should.

3. You should cache your selectors. You're querying for .event_owner a total of 7 times in your code, and an additional 4 times on every click.

4. Instead of listening for every click inside .event_owner, you should instead only listen for the change event of the radio buttons.

5. On page load, if there's a value in the input text field, all we have to do is check the correct radio button. The change event listener will take care of the rest.

With the above in mind, here's some sample code you might consider:

var $wrapper =$('.event_owner'),
$textInput =$wrapper.find('input[type="text"]'),
$radios =$wrapper.find('input[type="radio"]'),
owner = $textInput.val();$radios.on( 'change', function () {
$textInput.val($radios.filter(':checked').val() );
$radios.each(function () {$(this).closest('p').css( 'opacity', this.checked ? 1 : .3 );
});
});

if ( owner ) \$radios.filter('#' + owner).prop('checked', true);


Here's the fiddle: http://jsfiddle.net/Zf4UE/8/

-