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 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);
}
share|improve this question
add comment

1 Answer

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);
    
share|improve this answer
    
I wrote this code so long ago :) I've come a long way since then, but thanks for the notes. –  m59 Mar 7 at 15:45
1  
Yeah, I am hunting down zombie questions ;) –  konijn Mar 7 at 15:46
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.