1
\$\begingroup\$

I am trying to crank up my javascript knowledge by diving into the MVC/MV* design patterns in order to create more reusable and extendable apps.

I am currently sketching out a webapp that involves real time updating of various data feeds.(Facebook and Twitter in the example code)

The app uses jQuery, underscore and native code. I would love to get some feedback on my current implementation with regards to MV*/MVC. Am i doing it wrong/good etc.

Config code:

<script type="text/template" id="fb-list-template">
    <li>Entry: <%- app.title %></li>
</script>
<script type="text/template" id="tw-list-template">
    <li>Entry: <%- app.title %></li>
</script>
<script type="text/template" id="tw-stats-template">
    Total tweets: <%- app.tweets %>
    <br />
    Total lists: <%- app.lists %>
    <br />
    Total followers: <%- app.followers %>
</script>
<script>
    ;var AppApi = (function appConfig (AppApi, $, _, undefined) {

            'use strict';

            _.templateSettings.variable = 'app';

            AppApi.config = {
                baseurl : window.location.protocol + '//' + window.location.host,
                baseuri : window.location.pathname
            };

            AppApi.config.feeds = [{
                type : 'list',
                namespace : '_facebook_list_',
                api_url : AppApi.config.baseurl + '/assets/script/facebook_list_test.php',
                el : $('#fb-list'),
                template : $('#fb-list-template').html()
            }, {
                type : 'list',
                namespace : '_twitter_list_',
                api_url : AppApi.config.baseurl + '/assets/script/twitter_list_test.php',
                el : $('#tw-list'),
                template : $('#tw-list-template').html()
            }, {
                type : 'stats',
                namespace : '_twitter_stats_',
                api_url : AppApi.config.baseurl + '/assets/script/twitter_stats_test.php',
                el : $('#tw-stats'),
                template : $('#tw-stats-template').html()
            }];


            return AppApi;

        }(AppApi || {}, jQuery, _));
</script>

App code:

<script>
;var AppApi = (function appInit (AppApi, $, _, window, document, undefined) {

'use strict';

var ListModel = function (opts) {
    this.namespace = opts.namespace;
    this.api_url = opts.api_url;

    this.items = [];
};

ListModel.prototype = {

    getItems : function () {
        return [].concat(this.items);
    },

    addItem : function (item) {
        this.items.push(item);
        Observer.publish(this.namespace + 'item_added', item);
    },

    openConnection : function () {

        var _this = this;

        $.ajax({
            url : this.api_url,
            type : 'GET',
            dataType : 'json',
            success : function (response) {
                _this.addItem(response);
            },
            complete : function ($xhr, status) {
                _this.openConnection();
            },
            timeout : 50000
        });
    }
};

var StatsModel = function (opts) {
    this.namespace = opts.namespace;
    this.api_url = opts.api_url;

    this.stats = {};
};

StatsModel.prototype = {

    getStats : function () {
        return this.stats;
    },

    updateStats : function (stats) {
        this.stats = stats;
        Observer.publish(this.namespace + 'stats_updated', stats);
    },

    openConnection : function () {

        var _this = this;

        $.ajax({
            url : this.api_url,
            type : 'GET',
            dataType : 'json',
            success : function (response) {
                _this.updateStats(response);
            },
            complete : function ($xhr, status) {
                _this.openConnection();
            },
            timeout : 50000
        });
    }
};

var ListView = function (model, opts) {
    this.model = model;
    this.el = opts.el;
    this.template = _.template(opts.template);

    Observer.subscribe(this.model.namespace + 'item_added', this.addItem, this);
};

ListView.prototype = {

    addItem : function (item) {
        this.el.prepend(this.template(item));
    }
};

var StatsView = function (model, opts) {
    this.model = model;
    this.el = opts.el;
    this.template = _.template(opts.template);

    Observer.subscribe(this.model.namespace + 'stats_updated', this.updateStats, this);
};

StatsView.prototype = {

    updateStats : function (stats) {
        this.el.html(this.template(stats));
    }
};

var Observer = {

    subscribers : {
        all : []
    },

    subscribe : function (type, fn, context) {

        type = type || 'all';
        fn = typeof fn === 'function' ? fn : context[fn];

        if (typeof this.subscribers[type] === 'undefined') {
            this.subscribers[type] = [];
        }

        this.subscribers[type].push({ 
            fn : fn, 
            context : context || this 
        });
    },

    unsubscribe : function (type, fn, context) {
        this.updateSubscriptions('unsubscribe', type, fn, context);
    },

    publish : function (type, publication) {
        this.updateSubscriptions('publish', type, publication);
    },

    updateSubscriptions : function (action, type, arg, context) {

        var i,
        type = type || 'all',
        subscribers = this.subscribers[type],
        nrofsubscribers = subscribers ? subscribers.length : 0,
        subscriber;

        for (i = 0; i < nrofsubscribers; i += 1) {

            subscriber = subscribers[i];

            if (action === 'publish') {
                subscriber.fn.call(subscriber.context, arg);
            } else {
                if (subscriber.fn === arg && subscriber.context === context) {
                    subscribers.splice(i, 1);
                }
            }
        }
    }
};

if (!AppApi.config || !AppApi.config.feeds) {
    throw new Error('Please configure the feeds properly. Aborting...');
    return;
} else {

    var i,
    nroffeeds = AppApi.config.feeds.length,
    config;

    for (i = 0; i < nroffeeds; i+=1) {

        config = AppApi.config.feeds[i];

        if (config.type === 'list') {

            var model = new ListModel({
                api_url : config.api_url,
                namespace : config.namespace
            }),

            view = new ListView(model, {
                el : config.el,
                template : config.template
            });

            model.openConnection();

        } else if (config.type === 'stats') {

            var model = new StatsModel({
                api_url : config.api_url,
                namespace : config.namespace
            }),

            view = new StatsView(model, {
                el : config.el,
                template : config.template
            });

            model.openConnection();

        } else {
            throw new Error('Error. Aborting...');
        }
    }
}

return AppApi;

    }(AppApi || {}, jQuery, _, this, this.document));
</script>
\$\endgroup\$
  • \$\begingroup\$ The source can be found at this url. 001-030.nick.mdcdev.nl/app.php right click and view source. Below at the end of the body tag there is a script tag wich holds the config, the app is located in 001-030.nick.mdcdev.nl/assets/js/app.js \$\endgroup\$ – ngr Mar 28 '13 at 14:38
  • \$\begingroup\$ Please paste the code here with the question. \$\endgroup\$ – JJJ Mar 28 '13 at 15:00
  • \$\begingroup\$ Code has been added. Merci folks! \$\endgroup\$ – ngr Mar 28 '13 at 15:40
3
\$\begingroup\$

I think this code will require more than one review and more than one rewrite. Please consider the following: * If you are planning for more feeds, you could consider a helper function to start each one.

  for (i = 0; i < nroffeeds; i+=1) {

    config = AppApi.config.feeds[i];

    if (config.type === 'list') {

        var model = new ListModel({
            api_url : config.api_url,
            namespace : config.namespace
        }),

        view = new ListView(model, {
            el : config.el,
            template : config.template
        });

        model.openConnection();

    } else if (config.type === 'stats') {

        var model = new StatsModel({
            api_url : config.api_url,
            namespace : config.namespace
        }),

        view = new StatsView(model, {
            el : config.el,
            template : config.template
        });

        model.openConnection();

    } else {
        throw new Error('Error. Aborting...');
    }
}

Could become then

feedTypes =
{
  'list'  : { model : ListModel  , view : ListView },
  'stats' : { model : StatsModel , view : StatsView }
};
for (i = 0; i < nroffeeds; i+=1)
{
  config = AppApi.config.feeds[i];
  feedType = feedTypes[config.type];
  if( feedType )
  {
    var model = new feedType.mode( config ); //Not sure this is future proof
    var view = new feedType.view( model , config ); //Not sure this is future proof
    model.openConnection();
  }
  else
  { 
    throw new Error('Error. Aborting...');
  }
}
  • Also, openConnection() violates DRY. I would consider having a Model class with an openConnection function. And then use that function.

  • In general, consider having a parent class for Model and View. There is so much duplicated code

  • nrofsubscribers looks not nice, subscriberCount looks better

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your input! It brought me on some better ideas. Reviews and rewrites is how to learn writing better code, this way of writing js is new to me so any feedback is much appreciated. :) \$\endgroup\$ – ngr Mar 30 '13 at 23:33

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.