6
\$\begingroup\$

As a beginner I am always determined to improve myself. I've got written the following Code using jQuery that is working fine. However, I am sure that there are cleaner ways for achieving the same.

Right now, I am struggling with the this keyword and the asynchronous loading of the JSON file. Moreover, I am not sure whether you should call an function for initialization the way I did.

Do you have any suggestions for improvements?

$(function(){

    function Model() {
        this.data = null;
        this.init();
    };

    Model.prototype = {

        deferred: $.Deferred(),

        config: {
            jsonFile: 'dump.json'
        },

        init: function() {
            this.loadJson();
        },

        loadJson: function() {
            var self = this;
            jQuery.getJSON(
                this.config.jsonFile,
                function(data) {
                    self.data = data;
                    self.deferred.resolve();
                }
            )
            .fail(function() {
                console.error("Loading JSON file failed");
            })
            .always(function() {});
        },

        getData: function(callback) {
            console.log("getData")
            var self = this;

            $.when(this.deferred)
            .done(function() {
                callback(self.data)
            })
        },
    };

    var model = new Model();
    model.getData(function(data) {
        console.log(data)
    });

});

(duplicate of https://stackoverflow.com/q/23142089/3546614)

Update 1

I've just updated my code (and truncated unimportant stuff) taking @jgillich's advices into account. For the moment I feel better reading the JSON file when the object is generated which is why I outsourced the operation into separate function.

$(function(){
    function Model() {
        this.loadJson();
    };

    Model.prototype = {

        deferred: $.Deferred(),
        jsonFile: 'dump.json',

        loadJson: function() {
            $.getJSON(this.jsonFile)
                .done(function (data) {
                    this.data = data;
                    this.deferred.resolve(data);
                }.bind(this))
                .fail(this.deferred.reject);
        },

        getData: function() {
            return this.deferred.promise();
        },
    };

    var model = new Model();
    model.getData().done(function(data) {
        console.log('done')
        console.log(data)
    });
});
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$
  • Storing deferred in the model and then passing a callback to getData is a bit obscure. You could write it like this instead:

    getData: function() {
        var deferred = $.Deferred();
    
        if(!this.data) {
            $.getJSON('...',).done(function (data) {
                this.data = data;
                deferred.resolve(data);
            }.bind(this)).fail(deferred.reject);
        } else {
            deferred.resolve(this.data);
        }
    
        return deferred.promise();
    }
    
    model.getData().done(function (data) { /* ... */});
    
  • Use either $ or jQuery, don't mix them.

  • Use Function.prototype.bind instead of var self = this.

  • Use semicolons after each statement (or leave them out entirely if you prefer that).

  • this.data = null; seems pointless, newly created objects don't have such a property.

\$\endgroup\$
0

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.