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 and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.