Creating and referencing markup....cloning, etc. efficient system for this plugin

Question

I am pulling multiple feeds from youtube that have a callback function which makes markup from each feed. They add that markup to a documentFragment stored in var data. When all the feeds have contributed what they should, a final callback runs. Each feed can deal with its own thumbnails and page menu, so it makes sense to get that started without waiting for all of the feeds to be done. Some markup requires all of them to be done, so I do that in the final callback. To complicate matters, the markup needs to be appended in an order dictated by the user and some sets of markup may need to be cloned if requested. This allows the user to easily build the plugin as they see fit. Here is the option for that.

build:['listsMenu', 'listTitle', 'listDesc', 'player', 'pageMenu', 'thumbnails']

or another possibility...something like this:

build:['player', 'listsMenu', 'pageMenu', 'thumbnails', 'pageMenu']

This is the best solution I have been able to come up with so far. Any suggestions are appreciated!

//in the function that makes the ajax calls -
    data = {};

namespace.thumbs.prototype.feedCallback = function(context, options, data, optList, listNum, feed) {
    if (options.build.indexOf('pageMenu') !== -1 || options.build.indexOf('thumbnails') !== -1) {
        var feedVideosInPages = this.return_feedVideosInPages(options.thumbCount, options.thumbsPerPage, feed.videos);
        var pagesMenu = this.return_PagesMenu(options, listNum, feedVideosInPages);
        var thumbnails = this.return_thumbsMarkup(options, optList, listNum, feedVideosInPages);
        data.pagesMenus = data.pagesMenus || document.createDocumentFragment();
        data.thumbnails = data.thumbnails || document.createDocumentFragment();
        if (pagesMenu) { data.pagesMenus.appendChild(pagesMenu); }
        data.thumbnails.appendChild(thumbnails);
    }

    data.each = data.each || [];
    data.each.push({'feed':feed, 'optList':optList, 'listNum':listNum});

    if (data.each.length === options.lists.length) { this.allFeedsReady(context, options, data); }
};
namespace.thumbs.prototype.allFeedsReady = function(context, options, data) {
    this.sort_dataEach(data);

    elems = document.createDocumentFragment();
    addedElemNames = [];
    clonedElems = {};
    toAdd = options.build.slice();
    var buildLength = options.build.length;
    for (var i=0; i<buildLength; ++i) {
        var name = options.build[i];
        if (addedElemNames.indexOf(name) === -1) { //this is the first time the element has been added.
            switch (name) {
                case 'pageMenu': elem = data.pagesMenus; break;
                case 'thumbnails': elem = data.thumbnails; break;
                case 'player': elem = this.return_playerDiv(context); break;
                case 'listMenu': elem = this.return_listsMenuMarkup(data.each); break;
                //case 'listTitle': elem = allData.markup.listsTitles[0]; break;
                case 'listDesc': elem = this.return_listsDescsMarkup(data.each); break;
                case 'videoTitle': elem = this.return_videoTitleMarkup(data.each); break;
                    default: elem = false;
            }
            addedElemNames.push(name);
        }
        else {
            elem = clonedElems[name];
        }
        toAdd.splice(toAdd.indexOf(name), 1); //remove this item from array of things to be added
        if (elem) { 
            if (toAdd.indexOf(name) !== -1) { clonedElems[name] = elem.cloneNode(true); }//the build will contain another copy, so clone it
            elems.appendChild(elem); 
        }
    }
    context.appendChild(elems);
}

Answer 1

From a once over:

• data should be declared with var, so should a number of other variables. Please paste your code into the JSHint.com site and have a look

• It is considered better form to have 1 comma separated var statement:

  var feedVideosInPages = this.return_feedVideosInPages(options.thumbCount, options.thumbsPerPage, feed.videos),
      pagesMenu = this.return_PagesMenu(options, listNum, feedVideosInPages),
      thumbnails = this.return_thumbsMarkup(options, optList, listNum, feedVideosInPages);
  

• return_feedVideosInPages is a terrible name, I assume you chose it because feedVideosInPages was already taken ;) I would for something shorter:

  var feedVideos = this.getFeedVideosInPages(options.thumbCount, options.thumbsPerPage, feed.videos),
  

  which is still horizontally challenged, since you call that function only once, and since thumbCount and thumbsPerPage are in the same convenient options object you could improve even further to:

  var feedVideos = this.getFeedVideos(options, feed.videos),
  

  Finally, if you think about it, since you get the videos from the feed, let getFeedVidoes decide/worry about which property of feed contains the videos:

  var feedVideos = this.getFeedVideos(options, feed),
  

• Bad (TMI):

  if (toAdd.indexOf(name) !== -1) { clonedElems[name] = elem.cloneNode(true); }//the build will contain another copy, so clone it
  

  Better ( Comment on top ):

  //The build will contain another copy, so clone it
  if (toAdd.indexOf(name) !== -1) { clonedElems[name] = elem.cloneNode(true); }
  

  Even better ( never drop the newline, but you can drop the curlies! )

  //The build will contain another copy, so clone it
  if (toAdd.indexOf(name) !== -1)
    clonedElems[name] = elem.cloneNode(true);
  

  The ~ operator ( Bitwise NOT ) is great for comparing to -1:

  if (!~toAdd.indexOf(name))
    clonedElems[name] = elem.cloneNode(true);