1
\$\begingroup\$

I'm looking for some feedback on my implementation of MVC and functional inheritance (as described by Douglas Crockford in his book The Good Parts) in an app that uses the YouTube API.

This is my first time building anything remotely object-oriented so I'd really appreciate some advice on this app.

You can view the app in full here. Here are some relevant code snippets:

Model:

models.YTData = function(data) {
    var feed = data.feed,
        entries = feed.entry,
        length = entries.length,
        newData = {vids: []};

        for(var i = 0; i < length; i+=1) {
            var index = (i + 1) % 5,
                entry = entries[i],
                date = entry.published.$t,
                jsDate = new Date(date.substring(0, 4), date.substring(5, 7) - 1, date.substring(8, 10)),
                todayDate = new Date(),
                oneDay = 1000 * 60 * 60 * 24,
                views = entry.yt$statistics.viewCount,
                numOfDays = Math.ceil((todayDate.getTime() - jsDate.getTime()) / oneDay);

            if (entry.media$group.media$content !== undefined) {
                newData.vids.push({
                    title: entry.title.$t,
                    url:  entry.media$group.media$content[0].url,
                    index: index === 0 ? index + 5 : (i + 1) % 5,
                    views: controllers.addCommas('' + views),
                    date: '' + jsDate.getUTCDate() + ' ' + controllers.utilFunctions(jsDate.getMonth()).convertMonth() + ' ' + jsDate.getFullYear(),
                    average: controllers.addCommas('' + Math.round(views / numOfDays))
                });
            }

            else {
                continue;
            }
        }
    localStorage.vids = JSON.stringify(newData);
    return newData;
};

Controller:

controllers.request = function(obj) {
                var obj = obj || {};
                var that = {
                    base_url: 'https://gdata.youtube.com/feeds/api/standardfeeds',
                    max_results: 25,
                    start_index: obj.start_index || 1,
                    alt: 'alt=json-in-script',
                    country: obj.country ? '/' + obj.country : '',
                    genre: obj.genre ? '&category=' + obj.genre : '',
                    call_url: function() {
                        return that.base_url + that.country + '/on_the_web_Music' + '?max-results=' + 
                              that.max_results + '&start-index=' + that.start_index + '&' + 
                              that.alt + that.genre;
                    },
                    ajax: function() {
                        views.loading();
                        $.ajax({
                            type: 'GET',
                            url: that.call_url(),
                            dataType: 'JSONP',
                            error: function() {
                                views.serverError().hide();
                            },
                            success: function(data) {
                                var loading = views.loading;
                                views.loading = function() {};
                                if (data.feed.entry === undefined) {
                                    views.noResults().hide();
                                }

                                else {
                                    views.displayFirst(models.YTData(data)).pager(obj.genre, obj.country);
                                }
                                views.loading = loading;
                            }
                        });
                    }
                };
                return that;
    };

Views (showing an example of functional inheritance):

views.display = function(data) {
    var that = {
        vids: data.vids,
        condition: data.vids.length >= 5 ? 5 : data.vids.length,
        hideCurrentVids: function() {
            $('.playerContainer').animate({opacity: 0}, 500, function() {
                that.showVids.apply(that);
            });
        },

        showVids: function() {
                $('#vids').html(views.tplVids(data)).css('opacity', '1');
                $('#loader').css('display', 'none');
                for (var i = 0; i < this.condition; i += 1) {
                    views.loadVideo(this.vids[i].url, false, i + 1);
                    $('#player' + (i + 1))
                        .parent()
                        .parent()
                        .css({position: 'relative', top: 0})
                        .delay(i * 500)
                        .animate({opacity: 1}, 1000);
                }
                $('#videos').removeData('loading');
                $('#loader').css('display', 'none');
        }

    }
    return that;
};

views.displayFirst = function(data) {
    var that = views.display(data); // inherits from views.display
    that.length = data.vids.length;
    that.pagerArray = {pagerItems: []};
    that.counter = 0;
    that.pager = function(genre, country) {
        if (this.length > 5) {
            for (var i = 0; i < this.length; i += 5) {
                this.counter += 1;
                this.pagerArray.pagerItems.push({
                    index: this.counter
                });
            }
            document.getElementById('pager').innerHTML = views.pagerGen(this.pagerArray);
            $('#pager').animate({
                opacity: 1
            }, 500);
            data.vids.splice(5);
        }

        else {
            $('#pager').empty();
        }
        this.showVids();
        views.viewing(genre, country);
    };
    return that;
};
\$\endgroup\$
  • 1
    \$\begingroup\$ Your controller has SEVEN levels of indentation, if your levels of identation have gone over 4 then your doing it wrong, break your functions into smaller ones \$\endgroup\$ – Raynos Mar 31 '12 at 17:50
  • \$\begingroup\$ @Raynos Thanks for your response. Good point - I had thought my functions were a bit bloated. \$\endgroup\$ – Stephen Young Mar 31 '12 at 20:50
  • \$\begingroup\$ Sorry, I'm having a problem finding the functional inheritance bit in your view ... That's a lot of code :) \$\endgroup\$ – oligofren Nov 1 '13 at 13:04
2
\$\begingroup\$

I'm not a JavaScript expert, so just a few general notes:

  1. var index = (i + 1) % 5,
    ...
    index: index === 0 ? index + 5 : (i + 1) % 5,
    

    If I'm right, it could be written as:

    index: index === 0 ? 5 : index,
    
  2. I'd use a guard clause here:

    if (entry.media$group.media$content !== undefined) {
        newData.vids.push({
            ...
        });
    }
    else {
        continue;
    }
    

    So, it would look like this:

    if (entry.media$group.media$content === undefined) {
        continue;
    }
    
    newData.vids.push({
        ...
    });
    

    References: Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code; Flattening Arrow Code

  3. Instead of

    condition: data.vids.length >= 5 ? 5 : data.vids.length,
    

    use Math.min:

    condition: Math.min(data.vids.length, 5),
    

    I think it's easier to read.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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