3
\$\begingroup\$

I wrote a quick function to create a set of radio buttons, three per table row. I would like some feedback about how I can shorten this bit of code using algebraic functions, or different jQuery functionality.

var arr = [ "earth", "mars", "jupiter", "saturn", "venus", "argus", "pluto", "janus", "canary", "orange", "butter pineapple", "eventual bliss"];

var obj = { one:"earth", two:"mars", three:"jupiter", four:"saturn", five:"venus" };

var start = ""; end = "";
var count = 0;
$("#radio_container").append("<table>"); 
    $.each(arr, function(i, val) {

        if(count == 3)
        {
            start = "<tr>";
            end = "</tr>"
            count= 0;
        }

        $("#radio_container").append(start);    
        $("#radio_container").append("<td><input name='finalcategory' type='radio' value='"+val+"'></input></td><td class='style3'>"+val+"</td>");
        $("#radio_container").append(end);  
        count++;
        start = "";
        end = "";

    });
$("#radio_container").append("</table>");
\$\endgroup\$
5
  • \$\begingroup\$ Is this question suitable for CR.se? The code as is does not produce the intended result. \$\endgroup\$
    – Joseph Silber
    Commented Aug 9, 2012 at 19:59
  • \$\begingroup\$ The code was originally in a fiddle, and someone pulled it out of the fiddle and changed my CR question. Fiddle: jsfiddle.net/KNseM/1 \$\endgroup\$
    – Tony H.
    Commented Aug 9, 2012 at 20:08
  • 1
    \$\begingroup\$ Like I said, this code does not produce the intended result. Inspect the rendered DOM and you'll see what I mean. \$\endgroup\$
    – Joseph Silber
    Commented Aug 9, 2012 at 20:10
  • 1
    \$\begingroup\$ I cannot stress this enough: add labels to the text next to the radio buttons: <label><input type="radio" name="foo"> You can click the text to pick</label> \$\endgroup\$
    – ANeves
    Commented Aug 10, 2012 at 16:45
  • \$\begingroup\$ You should definitely follow @ANeves' advice. I added the code to my answer. \$\endgroup\$
    – Joseph Silber
    Commented Aug 16, 2012 at 17:04

2 Answers 2

Reset to default
2
\$\begingroup\$

Here's how I would probably tackle this:

var arr = [ "earth", "mars", "jupiter", "saturn", "venus", "argus", "pluto", "janus", "canary", "orange", "butter pineapple", "eventual bliss"];
var obj = { one:"earth", two:"mars", three:"jupiter", four:"saturn", five:"venus" };

// Create the table here and attach it to your document.
var $table = $('<table>').appendTo('#radio_container'),
    // also, create the scope of $tr so we can keep using it.
    $tr;
$.each(arr, function(k,v){
  // if it's a multiple of 3, create a new row and attach
  // it to the table
  if ((k % 3) == 0){
    $tr = $('<tr>').appendTo($table);
  }
  // create the data cell
  $('<td>',{
    // add a radio input element within
    'html':$('<input>',{
      'name':'finalcategory',
      'type':'radio',
      'value':v
    })
  }).appendTo($tr); // attach it to the row

  // create the second cell, style and populate it,
  // and also attach it to the row.
  $('<td>',{
    'class':'style3'
  }).text(v).appendTo($tr);
});

Working example, and the notes:

  1. Use jQuery to build the objects, don't use "<html>" strings.
  2. With a little refactoring, you can use the pre-created objects to your advantage and rid yourself of a counter variable.
  3. You can also use modulo (%) to figure out which row you're on.

Some other things:

  1. jQuery allows you to specify an object ({}) as a second parameter which will set the attributes of the object it's creating (see how I've done it to mimic your code).
  2. Use .text() whenever possible to avoid injecting foul information in to the markup. .text() will sanitize the data for you--let it.

EDIT Feel free to also heed the advice of a comment about not showing the table until it has completed rendering. This can be done very simply using .hide() and .show() as shown in this example

\$\endgroup\$
4
  • \$\begingroup\$ We can argue about whether string concatenation is the way to go, but regardless, you shouldn't be appending that table to the DOM until you're done messing around with it. \$\endgroup\$
    – Joseph Silber
    Commented Aug 9, 2012 at 19:57
  • 1
    \$\begingroup\$ I can't stress this enough: messing with the DOM is extremely expensive. Here's a performance test comparing both our methods: jsperf.com/dom-vs-concatentation \$\endgroup\$
    – Joseph Silber
    Commented Aug 9, 2012 at 20:23
  • \$\begingroup\$ Hiding and then showing the table doesn't help much. You're still messing with the live DOM! (Hiding/Showing might actually make it worse). I added your other method to the performance test: jsperf.com/dom-vs-concatentation \$\endgroup\$
    – Joseph Silber
    Commented Aug 9, 2012 at 20:27
  • \$\begingroup\$ @JosephSilber: Acknowledged. I'm not arguing performance, at all. I am however arguing against directly inserting textual information through concatenation. I just don't feel comfortable with "+v+". And, FWIW, my same code could handle a lot more variations in arr where as yours would fail. \$\endgroup\$
    – Brad Christie
    Commented Aug 9, 2012 at 20:30
4
\$\begingroup\$

Here are some points to consider:

  1. Your end variable is declared without a var, so that it's leaking into the global namespace. Don't do that.

  2. As it stands now, your obj isn't used anywhere in the code. I'll assume it's not applicable here.

  3. Instead of manually keeping count of your iteration, use the % (Modulus) operator to calculate your rows.

  4. jQuery's append method doesn't deal with code fragments the way you seem to think it does. append("<table>") actually appends a whole table, not just an opening tag (that's not even possible).

  5. Access to the DOM is not free. Every time you hit the DOM you incur some overhead. Try keeping DOM modification to a minimum.

  6. Unlike others, when dealing with a simple HTML construct such as this, I prefer to build my fragment from a concatenated string since it's much faster.

With all that in mind, here's how I would do it:

var arr = ["earth", "mars", "jupiter", "saturn", "venus", "argus", "pluto", "janus", "canary", "orange", "butter pineapple", "eventual bliss"],
    snippet = '<table><tr>';

$.each(arr, function(i, val) {

    if (i % 3 == 0) snippet += '</tr><tr>';

    snippet += '<td><input name="finalcategory" type="radio" value="' + val + '" /></td>' +
               '<td class="style3">' + val + '</td>';
});

snippet += '</tr></table>';

$("#radio_container").append( snippet );

See it here in action: http://jsfiddle.net/DWcSN/

The code above works, but the text is not associated with the radio button in any way.

To remedy that, consider wrapping labels around your radio buttons:

snippet += '<td><label class="style3">' +
               '<input name="finalcategory" type="radio" value="' + val + '" />' +
               val +
           '</label></td>';

See it here in action: http://jsfiddle.net/DWcSN/1/ (clicking the text activates the corresponding button).

P.S. style3 is not very descriptive. You should consider coming up with a more intuitive naming convention.

\$\endgroup\$
2
  • \$\begingroup\$ You know i learned alot from asking this one question. There is so much knowledge on this board, i cannot not begin to express how much i appreciate all the help. \$\endgroup\$
    – Tony H.
    Commented Aug 21, 2012 at 22:24
  • \$\begingroup\$ @AntonioHerrera - Glad you're learning! That's what we're all here for. \$\endgroup\$
    – Joseph Silber
    Commented Aug 21, 2012 at 22:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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