How to rewrite this function using jquery

Question

I'm trying to write shorter code using jquery. What is the best way to rewrite this function in jquery.

function searchDone(results) {
    var result = null;
    var parent = document.getElementById('resultList');
    parent.innerHTML = '';
    var child = null;

    for (var i = 0; i < results.SearchResponse.Image.Results.length; i++) {
        result = results.SearchResponse.Image.Results[i];
        child = document.createElement('li');
        child.className = "resultlistitem";

        child.innerHTML = '<a href="' + result.MediaUrl +'"><img src="' +
            result.Thumbnail.Url +'" alt="' + result.Title +'" /></a>';

        parent.appendChild(child);
    }
}

What does the data look like that you're getting back? Is it XML? — jamesmortensen
– jamesmortensen, Commented Jan 30, 2011 at 21:44

PetersenDidIt · Accepted Answer · 2011-01-30 23:43:23Z

1

Its a bad idea to continually append to the same element in the DOM in loop. It means that the DOM will get redrawn every single time you append a new element.

Do something like this to append only once:

function searchDone(results) {
 var list = [];
    $.each(results.SearchResponse.Image.Results, function(i, v) {
        list.push('<li class="resultlistitem"><a href="' + v.MediaUrl +'"><img src="' + v.Thumbnail.Url +'" alt="' + v.Title +'" /></a></li>');
    });
 $('#resultList').empty().append(list.join(''));
}

This creates an array will all the "children" in it. Once the each as looped over all the results it empties out the #resultList and appends all the children.

edited Jan 30, 2011 at 23:43

answered Jan 30, 2011 at 22:07

PetersenDidIt

25.6k3 gold badges69 silver badges72 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Šime Vidas Over a year ago

@Petersen +1 but you have to use the second argument - the first one is the index...

Hussein Over a year ago

yes @PetersenDidIt, your code still doesn't work. Needs some tweaking

PetersenDidIt Over a year ago

@alex oops looks like there was an extra ) hanging out after function(i, v) should work now.

Hussein Over a year ago

That works, Thanks for the efforts. @aSeptik has a similar answer with the exception that he forgot to empty before appending. Your answer is the winner.

3 revs · Accepted Answer · 2011-01-30 22:02:22Z

1

function searchDone(results) {
    var result = null;
    var item = '';
    $('#resultList').empty();
    for (var i = 0; i < results.SearchResponse.Image.Results.length; i++) {
        result = results.SearchResponse.Image.Results[i];
        item = '<li>' +
                 '<a href="' + result.MediaUrl + '"> ' +
                   '<img src="' + result.Thumbnail.Url + '" alt="' + result.Title + '"/>' + 
                 '</a>' +
               '</li>';
        $('#resultList').append(item);
   }
}

edited Jan 30, 2011 at 22:02

community wiki

3 revs
Jared Farrish

5 Comments

laher Over a year ago

Beat me to it! ... I'd also use the 'each' method in place of the for loop

Šime Vidas Over a year ago

@Jared You have to declare the local variables

Jared Farrish Over a year ago

@Sime: Can you explain what you mean, and the reason why?

Šime Vidas Over a year ago

@Jared You have two undeclared variables: result and item. If you don't declare them, they become implicit global variables which is something that you certainly don't want.

Jared Farrish Over a year ago

@amir75: I left that the way it was since it wasn't really related to jQuery. If I were to rewrite that part, I might use do/while instead. I don't think it ALL has to be jQuery.

Luca Filosofi · Accepted Answer · 2011-01-30 22:27:59Z

1

If you dont care about effects in the sense of "display one message one by one" you can display it all together... like this

function searchDone(results) {
    var lis = [];
    $.each(results.SearchResponse.Image.Results, function(i,item){
    lis.push('<li class="resultlistitem"><a href="' + item.MediaUrl +'"><img src="' +
            item.Thumbnail.Url +'" alt="' + item.Title +'" /></a></li>');
    }); $('#resultList').html( lis.join('') );
}

otherwise with a little bit of code you can have a nice fadeOut effect like this:

DEMO: http://jsbin.com/odimi4

function searchDone(results) {
    $.each(results.SearchResponse.Image.Results, function(i,item) ) {
        $('<li class="resultlistitem">').html('<a href="' + item.MediaUrl +'">'+
          '<img src="' +item.Thumbnail.Url +'" alt="' + item.Title +'" /></a>')
          .appendTo('#resultList').delay(i+'000').fadeOut(200,function(){$(this).remove()});
    }); 
}

edited Jan 30, 2011 at 22:27

answered Jan 30, 2011 at 21:55

Luca Filosofi

31.2k9 gold badges72 silver badges77 bronze badges

2 Comments

Šime Vidas Over a year ago

You are querying for the '#resultList' element in each loop iteration. I'd be better if that element was cached.

Šime Vidas Over a year ago

Also, the first argument of the $.each callback is the index, not the value (which is the second parameter)

Šime Vidas · Accepted Answer · 2011-01-30 23:40:29Z

1

function searchDone(results) {
    var r,
        p = $('#resultList').empty(),
        arr = results.SearchResponse.Image.Results;

    for (var i = 0; i < arr.length; i++) {
        r = arr[i];
        p.append('<li class="resultlistitem"><a href="' + r.MediaUrl + '"><img src="' + r.Thumbnail.Url + '" alt="' + r.Title + '" /></a><li>');
    }   
}

As @Petersen already pointed out, DOM manipulation should be avoided inside loops. The alternative code below will create the HTML source code string via the loop, and the append the whole string at once.

function searchDone(results) {
    var r,
        p = $('#resultList').empty(),
        arr = results.SearchResponse.Image.Results,
        s = '';

    for (var i = 0; i < arr.length; i++) {
        r = arr[i];
        s += '<li class="resultlistitem"><a href="' + r.MediaUrl + '"><img src="' + r.Thumbnail.Url + '" alt="' + r.Title + '" /></a><li>';
    }

    p.append(s);
}

edited Jan 30, 2011 at 23:40

answered Jan 30, 2011 at 21:53

Šime Vidas

187k66 gold badges290 silver badges392 bronze badges

2 Comments

PetersenDidIt Over a year ago

This will mean that you force the DOM to redraw every single time it loops, if this is a large list then the browser will freeze up while it loops and appends

Hussein Over a year ago

short and clean @sime vidas. But @eseptic made better using of jquery. +1

Michal · Accepted Answer · 2011-01-31 00:01:26Z

1

Assuming your results are JSON

    function searchDone(results) {
     $("#resultList").empty();
     html = "";

//creating this as a string and appending at the end is faster than appending each element 'live' - depends how many search results you have
    $.each( results.SearchResponse.Image.Results, function(key,result){
      html +='<li><a href="' + result.MediaUrl +'"><img src="' +
                result.Thumbnail.Url +'" alt="' + result.Title +'" /></a></li>';
     });
     $("#resultList").append(html);
    }

edited Jan 31, 2011 at 0:01

answered Jan 30, 2011 at 22:00

Michal

13.7k4 gold badges38 silver badges33 bronze badges

1 Comment

Hussein Over a year ago

your function(result) should be function(i, result).

Collectives™ on Stack Overflow

How to rewrite this function using jquery

5 Answers 5

4 Comments

5 Comments

2 Comments

2 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

4 Comments

5 Comments

2 Comments

2 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related