current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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();
}
share|improve this question

1 Answer 1

up vote 1 down vote accepted

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");
    }
};
share|improve this answer
    
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. –  CrimsonChin Sep 4 '12 at 16:20
    
one thing though sinsided the hasOwnProperty functions should they be something like dfd = this._handleData otherwise the dfd object gets lost. –  CrimsonChin Sep 4 '12 at 16:23
    
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. –  Larry Battle Sep 4 '12 at 16:32
    
have I mentioned that this is awesome? –  CrimsonChin Sep 13 '12 at 15:25
    
Out of interest, Parser is a constructor function, do you see anything wrong with it returning a promise rather than an instance of itself? –  CrimsonChin Sep 14 '12 at 13:15

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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