current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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?

share|improve this question

3 Answers 3

up vote 1 down vote accepted

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).

share|improve this answer
    
this will only work if the item stops at f and not z – SaggingRufus Jan 8 '14 at 14:23
    
I do not understand your comment, what do you mean? – konijn Jan 8 '14 at 14:32
    
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 – SaggingRufus Jan 8 '14 at 15:10
    
@SaggingRufus I either still misunderstand or you are mistaken. item could have hundreds of properties and this would still work. – konijn Jan 8 '14 at 16:34
    
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. – SaggingRufus Jan 8 '14 at 16:53
up vote 3 down vote

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!

share|improve this answer
    
stackoverflow.com/questions/227552/… just add the continuation character as I did sorry that was an over sight on my part – SaggingRufus Jan 8 '14 at 13:59
    
also that TR should all be on the same line. I cant get it to work in that code block – SaggingRufus Jan 8 '14 at 14:08
    
+1 sagging thanks but I liked the other answer more – Change Jan 8 '14 at 16:27
    
just realized i can't up vote as i don't have 15 reputation yet – Change Jan 8 '14 at 16:28
up vote 0 down vote

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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