Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have this code I am using in a private project. The following is a snippet of code I use to retrieve and load playlists from YouTube:

for (feedEntry in a.feed.entry) {
  if (a.feed.entry.hasOwnProperty(feedEntry)) {
  feedTitle = a.feed.entry[feedEntry].title.$t.replace('] - ', ']').replace(' : ', '').replace(/\(.*?\)/g, "").replace(/\[.*?\]/g, "");
  feedURL = a.feed.entry[feedEntry].link[1].href;
  fragments = feedURL.split("/");
  videoID = fragments[fragments.length - 2];
  url = videoURL + videoID;
  var c = feedTitle.split(' - ');
  tracklist.push({
    'title': c[1],
    'artist': c[0],
    'url': 'http://youtubeinmp3.com/fetch/?video=http://www.youtube.com/watch?v=' + videoID + '',
    'album': 'YouTube'
  })
}

This code, understandably, causes the browser to freeze a few seconds when loading larger playlists.

What ways could I go about optimizing this?

share|improve this question

migrated from stackoverflow.com Feb 16 at 14:34

This question came from our site for professional and enthusiast programmers.

    
You could look at using better regular expressions as they are very fast. –  Drewness Feb 16 at 14:25
1  
and you might want to do this on the server where you have more control - if you are reorganising data into an array all the time, I's vote to let the server do it ( you can cache it in a db ) –  Rob Sedgwick Feb 16 at 14:28
1  
@Praveen Oh, thanks! And i was suspecting I should do something about my expressions, but I picked up regexp yesterday so I'm not very good with them. –  Jacob Pedersen Feb 16 at 14:33
add comment

2 Answers 2

The freezing comes primarily from the fact that JavaScript is single-threaded so when your loop is busy-looping everything else will wait until the scope has finished before the browser can continue to poll on the event queue.

To solve this you can convert your code to asynchronous executing using setTimeout() to allow the browser to do other tasks in-between.

Another thing is that for in loops are very slow so this can be replaced by using Object keys instead.

Your parsing can be optimized by caching the current object as well as re-factor the parsing itself to a separate function method:

var keys = Object.keys(a.feed.entry),  // retrieve all keys from the object
    count = keys.length,               // number of keys to parse
    segment = 10,                      // number to parse each segment
    current;                           // counter for segment

// start loop
(function doSegment() {

    current = segment;                 // reset segment counter for each iteration

    while(current-- && count)
        parseFeedEntry(a.feed.entry[keys[--count]]);

    if (count)
        setTimeout(doSegment, 11);     // delay 11ms to allow browser to breath
})();

function parseFeedEntry(entry) {
    // parse the entry here
    var feedTitle = entry.title.$t.replace('] - ', ']'). ...8X...
    // etc., notice entry is now cached
}

Online demo

Hope this helps!

share|improve this answer
    
That's neat, but the freezing doesn't really affect this situation - I'm more looking to speed up the whole thing, thus I wanted to improve my expressions. Should I create a separate thread for that? –  Jacob Pedersen Feb 16 at 17:14
    
@JacobPedersen you can't make a separate thread unless you want to involve a web worker. In any case that would probably be slower than direct expression due to the memory copy between the two threads - and a bit overkill for something like this IMO. I'm not an ace at regex so I can't help there (and it wasn't tagged with expressions?). The gain is minimal though and the main issue with the code is the blocking code (for-in loop). The answer above shows a non-blocking approach which will solve that so the expressions themselves won't matter so much really. my 2 cents. –  Ken Fyrstenberg Feb 16 at 17:20
    
Alright, will try it out. By "Seperate thread" I meant seperate question :P –  Jacob Pedersen Feb 16 at 17:28
    
@JacobPedersen aah! :D My head is a bit tired today –  Ken Fyrstenberg Feb 16 at 17:33
    
Happens to the best of us :p Could you point out where/how i should put this? Here's my full function: jsfiddle.net/hRP35 –  Jacob Pedersen Feb 16 at 18:09
show 3 more comments

I found a better solution to the playlist loader!

My previous code is pretty messy (and buggy), but this is pretty clean and nice! (old: http://jsfiddle.net/hRP35/)

Here's the code:

//Loads playlist data from youtube
function ytpl(a){console.log(a);}

function loadPlaylist(a) {
   var script = document.createElement('script');
   script.src = a;
   var head = document.getElementsByTagName("head")[0];
   head.appendChild(script);
}
loadPlaylist('https://gdata.youtube.com/feeds/api/playlists/UUJ6td3C9QlPO9O_J5dF4ZzA?max-results=50&start-index=1&v=2&alt=json&callback=ytpl');

Fiddle: http://jsfiddle.net/P5S2Q/1/

Now I just need to fix my expressions and possibly make the for-loop asynchronous.

Thanks to anyone who helped!

share|improve this answer
add comment

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.