Is there a simpler way to do this script?

Question

I'm learning and trying to put together a little bit of jquery. Admittedly I'm finding it difficult to find a good basics guide, particularly, when adding multiple actions to one page.

I read somewhere that the document listener should only be used once. I believe I'm using it twice here, but not 100% sure how to bring it into one listener.

Also because I've been hacking bits of script together, I think I'm using parts of javascript and parts of jQuery. Is this correct?

A critique of the code below [which does work] and any advice on how best to approach learning jQuery would be most helpful. Thanks.

Script 1 styles a group of 3 radio buttons depending on which one is clicked. Script 2 appends new inputs to the bottom of a form.

var stateNo = <?php echo $HighestPlayerID; ?> + 1; 

$(document).on('click', 'input', function () {
    var name = $(this).attr("name");
    if ($('input[name="' + name + '"]:eq(1)')[0].checked) {
        $('label[name="' + name + '"]:eq(1)').addClass('nostate');
        $('label[name="' + name + '"]').removeClass('selected');
    }
    else {
        $('label[name="' + name + '"]').removeClass('nostate selected');
        if ($('input[name="' + name + '"]:eq(0)')[0].checked) {
            $('label[name="' + name + '"]:eq(0)').addClass('selected');
        }
        else {
            $('label[name="' + name + '"]:eq(2)').addClass('selected');
        }
    }
});


$(document).on('click', 'button[name=btnbtn]', function () {
    var stateName = 'state[' + stateNo + ']';
    var newPlayerAppend = `` +
        `<tr><td>` + 
        `<input type="hidden" name="state['` + stateNo + `'][PlayerID]" value="` + stateNo + `" /></td>` + 
        `<td><input name="` + stateName + `[Name]" value="Name"></td>` + 
        `<td><input name="` + stateName + `[Team]" type="radio" value="A">` +
        `<td><input name="` + stateName + `[Team]" type="radio" value="">` +
        `<td><input name="` + stateName + `[Team]" type="radio" value="B">` +
        `</td></tr>`;
    $("tbody").append(newPlayerAppend);
    stateNo++;
});

HTML for the 3 radio button inputs

<td class="Choice">
<label name="state[1][Team]" class="teampick Astate ">A
<input name="state[1][Team]" type="radio" value="A" />
</label>
<label name="state[1][Team]" class="smallx nostate ">X
<input name="state[1][Team]" type="radio" value="" checked />
</label>
<label name="state[1][Team]" class="teampick Bstate">B
<input name="state[1][Team]" type="radio"  value="B" />
</label>
</td>

Answer 1

It is absolutely fine to have multiple listeners but I usually prefer making everything under one roof. Consider making code as simple as possible which saves lot of time during maintenance.

you can use $(function() {}) or document.ready().

$(document).ready(function() {

    $('input[type="radio"]').click(function() {
     var thisa = $(this).parent();
     var name = $(this).attr("name");
     // Remove :selected class from the previous selected labels.
      $('label[name="' + name + '"]').removeClass('selected');
      // Add conditional class with tenary operator.
      thisa.parent().hasClass("smallx") ? thisa.addClass('nostate') : thisa.addClass('selected');
    });

    $('button[name=btnbtn]').click(function() {
         var stateName = 'state[' + stateNo + ']';
            // Add TR and TD before appending the row to tbody
            var newPlayerAppend = `<tr><td>` + 
                `<input type="hidden" name="state['` + stateNo + `'][PlayerID]" value="` + stateNo + `" />` + 
                `<input name="` + stateName + `[Name]" value="Name">` + 
                `<input name="` + stateName + `[Team]" type="radio" value="A"></td></tr>`;

            $("tbody").append(newPlayerAppend);
            stateNo++;
    });
 });

Hope this helps.

Answer 2

Some of the code can be written more concisely, or more the jQuery way, but first I want to highlight an issue with your current solution:

The following would generate invalid HTML, if it were not that browsers try to solve the inconsistency:

$("tbody").append(newPlayerAppend);

A tbody element cannot have input elements as direct children. If you really want the added content to be part of the table, you need to add a row and a cell, and put the new input elements in there.

Here is the code I would suggest, that does approximately the same as your code:

$(document).on('click', 'input', function () {
    $('label[name="' + $(this).attr('name') + '"]')
        .removeClass('nostate selected')
        .has(':checked')
        .addClass(function () {
            return $(this).is('.smallx') ? 'nostate' : 'selected';
        });
});

$(document).on('click', 'button[name=btnbtn]', function () {
    $('tbody').append($('<tr>').append($('<td>').append(
        $('<input>').attr({name: `state[${stateNo}][PlayerID]`, value: stateNo, type: 'hidden'}),
        $('<input>').attr({name: `state[${stateNo}][Name]`, value: 'Name'}),
        $('<input>').attr({name: `state[${stateNo}][Team]`, value: 'A', type: 'radio'})
    )));
    stateNo++;
});

There is no issue in having two handlers. They deal with different target elements, and even if they would deal with the same elements, it would still not be a real problem: the DOM is designed to deal with multiple event handlers.

Answer 3

1. There are 2 places you are using anonymous functions. If the code block moves to a named function, the entire code becomes more maintainable. It also helps better in debugging by telling you upfront which function name the error may lie in.

2. Once you have named functions you will realise that you really do have 2 event listeners for click. So there isn't much benefit of moving them in one listener (or one function you may be referring to). These both event listeners attach on document object and listen to a click event.

3. Class names are always better when hyphenated. a-state over Astate.

4. If it works it is correct code, for once you asked about correctness.