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

Please help reviewing my jQuery functions.


<div class="event_owner">
        <label for="xxx">xxx</label>
        <input type="radio" name="event_owner" id="xxx">

        <label for="yyy">yyy</label>
        <input type="radio" name="event_owner" id="yyy">
<input type="text" name="_event_owner[name]" autocomplete="off" value="">


    $(".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"});




  • 3
    \$\begingroup\$ 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. \$\endgroup\$ – Flambino Nov 6 '12 at 3:13

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/

| improve this answer | |

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.