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;
    $.each(arr, function(i, val) {

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

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

  • \$\begingroup\$ Is this question suitable for CR.se? The code as is does not produce the intended result. \$\endgroup\$ 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.
    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\$ 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
    Aug 10, 2012 at 16:45
  • \$\begingroup\$ You should definitely follow @ANeves' advice. I added the code to my answer. \$\endgroup\$ Aug 16, 2012 at 17:04

2 Answers 2


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.
$.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
    // add a radio input element within
  }).appendTo($tr); // attach it to the row

  // create the second cell, style and populate it,
  // and also attach it to the row.

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

  • \$\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\$ 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\$ 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\$ 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\$ Aug 9, 2012 at 20:30

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 +

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.

  • \$\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.
    Aug 21, 2012 at 22:24
  • \$\begingroup\$ @AntonioHerrera - Glad you're learning! That's what we're all here for. \$\endgroup\$ Aug 21, 2012 at 22:33

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.