Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

Please help reviewing my jQuery functions.

HTML

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

    <p>
        <label for="yyy">yyy</label>
        <input type="radio" name="event_owner" id="yyy">
    </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/

share|improve this question
3  
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
add comment

1 Answer

up vote 2 down vote accepted

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/

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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