3
\$\begingroup\$

I am getting data from JSON and then create rows of it:

        $.each(data.GetwebMethodResult, function (index, item) {
        $("#TableABC").append(FunABC(item.aID, item.b, item.c, item.d, item.e, item.f));
        });

function FunABC(a, b, c, d, e, f) {
    var row =
        "<tr class='Row' onclick=\"AnotherUnrelatedFunction('" + a + "' , '" + b + "', '" + c + "', '" + d + "', '" + e + "', '" + f + "')\">" +
        "   <div class='mr'>" +
        "       <td class='mc mci'>" +
        "           <div class='mcit'>" + a + "</div>" +
        "       </td>" +
        "   </div>" +
        "</tr>";

    return row;
}

Is there any better way of doing it?

\$\endgroup\$
1
\$\begingroup\$

I would think so;

  • encoding listeners through HTML onclick is old skool
  • variable names a,b,c,d,e,f earn an award for 2014 worst variable names
  • instead of sending just the item, you are sending every property you need.
  • you are setting content and listeners in one place ( mixing view and controller )
  • You have a div between tr and td, which should not even work

I would counter-propose something like

$.each(data.GetwebMethodResult, function (index, item)
{
  $("#TableABC").append( $(generateRow(a)).click( function(){ 
    anotherUnrelatedFunction( item );
  }));
});

function generateRow(a) 
{
  return '<tr class="Row mr">' + 
           '<td class="mc mci">' + 
             '<div class="mcit">' +
               a + 
             '</div>' +
           '</td>' + 
         '</tr>';
}

This basically keeps the HTML stringing separate from the onClick handler assignment. Through closure it passes the entire item object to anotherUnrelatedFunction which means the signature becomes more concise (you would have to adjust anotherUnrelatedFunction obviously).

\$\endgroup\$
  • \$\begingroup\$ this will only work if the item stops at f and not z \$\endgroup\$ – SaggingRufus Jan 8 '14 at 14:23
  • \$\begingroup\$ I do not understand your comment, what do you mean? \$\endgroup\$ – konijn Jan 8 '14 at 14:32
  • \$\begingroup\$ since he chose it as answer it much not be the case. But if there were more entires in item you would not be able to pull the whole thing \$\endgroup\$ – SaggingRufus Jan 8 '14 at 15:10
  • \$\begingroup\$ @SaggingRufus I either still misunderstand or you are mistaken. item could have hundreds of properties and this would still work. \$\endgroup\$ – konijn Jan 8 '14 at 16:34
  • \$\begingroup\$ yes but if not all of them were supposed to be transfer. For example: if item contain a-z but only a-f were required. \$\endgroup\$ – SaggingRufus Jan 8 '14 at 16:53
3
\$\begingroup\$

Not really. It hard to optimize something with little logic. You could do this instead:

var row =
    "<tr class='Row' onclick=\"AnotherUnrelatedFunction('" + a + "' , '" + b + "', '" + c + "', '" + d + "', '" + e + "', '" + f + "')\> \
       <div class='mr'> \
           <td class='mc mci'> \
               <div class='mcit'>" + a + "</div> \
           </td> \
       </div> \
    </tr>";

But aside from that looks good!

\$\endgroup\$
  • \$\begingroup\$ stackoverflow.com/questions/227552/… just add the continuation character as I did sorry that was an over sight on my part \$\endgroup\$ – SaggingRufus Jan 8 '14 at 13:59
  • \$\begingroup\$ also that TR should all be on the same line. I cant get it to work in that code block \$\endgroup\$ – SaggingRufus Jan 8 '14 at 14:08
0
\$\begingroup\$

Since you didn't specifically state you didn't want to use a third party library, I am going to recommend you look at underscore and it's template function. There are other template engines (mustache.js also comes to mind). These excel at doing exactly what you are trying to do.

\$\endgroup\$

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.