HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Set of radio buttons

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
buttonsradioset

Problem

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

        if(count == 3)
        {
            start = "";
            end = ""
            count= 0;
        }

        $("#radio_container").append(start);    
        $("#radio_container").append(""+val+"");
        $("#radio_container").append(end);  
        count++;
        start = "";
        end = "";

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

Solution

Here are some points to consider:

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

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

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

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

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

-
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 = '';

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

if (i % 3 == 0) snippet += '';

snippet += '' +
'' + val + '';
});

snippet += '';

$("#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 += '' +
               '' +
               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.

Code Snippets

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

Context

StackExchange Code Review Q#14497, answer score: 4

Revisions (0)

No revisions yet.