3
\$\begingroup\$

I have written a parser function that returns a JQuery Promise. You can see from the code that this is the top level parser and it delegates out to two other more specific parsers.

At the minute it feels a little scattered about with the promise being rejected and resolved all over the place. perhaps it would be better with a try and catch and an error handler that rejected the promsie? how would others approach this?

/**
 * Inspects JSON and delegates to either the sourceParser or the dataParser.
 * 
 * @constructor 
 * @param {Object} json
 * 
 * @returns JQuery Promsise
 *      done:       -
 *      fail:       error message
 *      progress:   Progress message
 */
function Parser(json)
{
    var dfd = $.Deferred();

    dfd.notify("Parsing");

    if (json && typeof json === "object")
    {           
        if (json.hasOwnProperty("GetDataSources"))
        {           
            var dataSource = json.GetDataSources;

            if (this.isSuccessfulResponse(dataSource.Response))
            {
                // Notify the caller of any progress
                dfd.notify("Parsing Source");

                // Create a new Source Parser
                var sourceParser =  new SourceParser(dataSource);
                sourceParser.done(dfd.resolve);
                sourceParser.fail(dfd.reject);
            }
            else
            {
                dfd.reject("Parsing Source Failed");
            }
        }
        else if (json.hasOwnProperty("GetData"))
        {           
            var data = json.GetData;

            if (this.isSuccessfulResponse(data.Response))
            {
                // Notify the caller of any progress
                dfd.notify("Parsing Data");

                // Create a new Data Parser
                var dataParser = new DataParser(data);
                dataParser.done(dfd.resolve);
                dataParser.fail(dfd.reject);
            }
            else
            {
                // Pass back an error message?
                dfd.reject("Parsing Data Failed");
            }
        }
    }
    else
    {
        dfd.reject("There was a problem reading the JSON")
    }

    return dfd.promise();
}
\$\endgroup\$
1
\$\begingroup\$

Chop up functions longer than 12 lines of code into smaller functions. Since you're using this, then you can attach the additional functions to the prototype on Parser.

Here's what I came up with.

function Parser(json) {
    var dfd = $.Deferred();
    dfd.notify("Parsing");
    if (json && typeof json === "object") {
        if (json.hasOwnProperty("GetDataSources")) {
            this._handleData( dfd, json.GetDataSources, SourceParser, "Source" );
        } else if (json.hasOwnProperty("GetData")) {
            this._handleData( dfd, json.GetData, DataParser, "Data" );
        }
    } else {
        dfd.reject("There was a problem reading the JSON");
    }
    return dfd.promise();
}
Parser.prototype._handleData = function(dfd, data, OtherParser, type){
    if (data && this.isSuccessfulResponse(data.Response)) {
        dfd.notify("Parsing " + type );
        var otherParser = new OtherParser(data);
        otherParser.done(dfd.resolve);
        otherParser.fail(dfd.reject);
    } else {
        dfd.reject("Parsing "+ type +" Failed");
    }
};
\$\endgroup\$
  • \$\begingroup\$ Thanks Larry. I had improved the code since posting it here but your approach is even better still. I have a tendency to create promises on top of promises unnecessarily! and I liked passing the parser as an Argument. \$\endgroup\$ – JonWells Sep 4 '12 at 16:20
  • \$\begingroup\$ one thing though sinsided the hasOwnProperty functions should they be something like dfd = this._handleData otherwise the dfd object gets lost. \$\endgroup\$ – JonWells Sep 4 '12 at 16:23
  • \$\begingroup\$ It's not really needed because objects are passed by reference and primitive values are passed by value in javascript. But I like returning the value of dfd to check the output since dfd is a private member. However, if return dfd; causes confusion then take it out. \$\endgroup\$ – Larry Battle Sep 4 '12 at 16:32
  • \$\begingroup\$ have I mentioned that this is awesome? \$\endgroup\$ – JonWells Sep 13 '12 at 15:25
  • \$\begingroup\$ Out of interest, Parser is a constructor function, do you see anything wrong with it returning a promise rather than an instance of itself? \$\endgroup\$ – JonWells Sep 14 '12 at 13:15

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.