Cleaning up Spaghetti Code w/ Promises

Question

In one of my beforeSave functions, I need to check a number of conditions before responding with success or error.

The problem is, my code seems really messy and sometimes success/error isn't called:

Parse.Cloud.beforeSave("Entry", function(request, response) {
    var entry = request.object;
    var contest = request.object.get("contest");

    entry.get("user").fetch().then(function(fetchedUser) {
      contest.fetch().then(function(fetchedContest) {
            if ( fetchedUser.get("fundsAvailable") < fetchedContest.get("entryFee") ) {
              response.error('Insufficient Funds.');
            } else {
                fetchedContest.get("timeSlot").fetch().then(function(fetchedTimeSlot) {
                    var now = new Date();
                    if (fetchedTimeSlot.get("startDate") < now) {
                        response.error('This contest has already started.');
                    } else {
                        contest.increment("entriesCount");
                        contest.increment("entriesLimit", 0); //have to do this, otherwise entriesLimit is undefined in save callback (?)

                        contest.save().then(function(fetchedContest) {
                            if (contest.get("entriesCount") > contest.get("entriesLimit")) {
                                response.error('The contest is full.');
                            } else {
                                response.success();
                            }
                        });
                    }
                });
            }
        });
    });
});

I've been trying to learn promises to tidy this up, and here was my (failed) attempt:

Parse.Cloud.beforeSave("Entry", function(request, response) {
    var entry = request.object;
    var contest = request.object.get("contest");

    entry.get("user").fetch().then(function(fetchedUser) {
        contest.fetch().then(function(fetchedContest) {
            if ( fetchedUser.get("fundsAvailable") < fetchedContest.get("entryFee") ) {
              response.error('Insufficient Funds.');
            }
            return fetchedContest;
        });
    }).then(function(result) {
        result.get("timeSlot").fetch().then(function(fetchedTimeSlot) {
            var now = new Date();

            if (fetchedTimeSlot.get("startDate") < now) {
                response.error('This contest has already started.');
            }
        });
    }).then(function() {
        contest.increment("entriesCount");
        contest.increment("entriesLimit", 0);
        contest.save().then(function(fetchedContest) {
            if (contest.get("entriesCount") > contest.get("entriesLimit")) {
                response.error('The contest is full.');
            }
        });
    }).then(function() {
        response.success();
    }, function(error) {
        response.error(error);
    });
});

Any help or examples on how to use promises for this would be much appreciated. Clearly I do not fully understand how they work syntactically yet.

It will be much easier to help if you describe the data. The best way to do that is to paste header rows from the data browser for each object (wide enough to include the columns referred to in the question). — danh, Jan 28 '16 at 01:42
To put the question and the answers offered into some sort of context, you might like to read **[How do I access previous promise results in a .then() chain?](http://stackoverflow.com/questions/28250680/)**. The question's original code is written in **[Nesting (and) closures](http://stackoverflow.com/a/28250687/3478010)** style, while the attempted tidy-up doesn't really follow any of the suggested patterns. Both answers below are **[Mutable Contextual State](http://stackoverflow.com/a/28250700/3478010)**, which, for the reasons given, should really be avoided. — Roamer-1888, Jan 28 '16 at 17:39
Personally, I would either stick with your original nested solution or adopt **[Break the chain](http://stackoverflow.com/a/28250704/3478010)**, in which, after a few lines of preamble, you would end up with a simple control structure of the form `Parse.Promise.when(p1, p2, p3).then(function(r1, r2, r3) {...}).then(...);`. I think that's about as neat as you could make it. If you are still interested, I'll post a full answer. — Roamer-1888, Jan 28 '16 at 17:39

score 1 · Answer 1 · answered Jan 28 '16 at 01:49

You are doing the rookie mistake of not returning promises inside .then while chaining them, other than that your chain would still continue even when you call response.error(.., simplest way to break chain is throwing an error, your code can be flattened as:

Parse.Cloud.beforeSave("Entry", (request, response) => {
  var entry = request.object,
   , contest = request.object.get("contest")
   , fetchedUser
   , fetchedContest;

  entry.get("user").fetch()
    .then(_fetchedUser => {
      fetchedUser = _fetchedUser;
      return contest.fetch();
    }).then(_fetchedContest => {
      fetchedContest = _fetchedContest;
      if ( fetchedUser.get("fundsAvailable") < fetchedContest.get("entryFee") ) 
        return Promise.reject('Insufficient Funds.');      
      return fetchedContest.get("timeSlot").fetch();
    }).then(fetchedTimeSlot => {
      var now = new Date();
      if (fetchedTimeSlot.get("startDate") < now) 
        return Promise.reject('This contest has already started.');      
      contest.increment("entriesCount");
      contest.increment("entriesLimit", 0); //have to do this, otherwise entriesLimit is undefined in save callback (?)
      return contest.save();
    }).then(fetchedContest => {
      if (contest.get("entriesCount") > contest.get("entriesLimit")) 
        return Promise.reject('The contest is full.');          
      response.success();          
    }).catch(response.error.bind(response));

});

I am indeed a rookie w/ JS. Thank you for explaining, however I am a bit confused by some of your syntax. So far I am only familiar with writing basic JS for Parse cloud code; I haven't seen the use of syntax like "=>" as you are using it here. Is that something I should be aware of? — vikzilla, Jan 28 '16 at 02:05
@vikzilla, I meant no disrespect(, meant people new to Promises), `=>` is called arrow function, shorthand for anonmyous functions, part of ES6/ ES2015. If you want to stick to ES5, replace ` a => {...}` with `function(a){...}` — mido, Jan 28 '16 at 02:22

score 1 · Accepted Answer · answered Jan 28 '16 at 02:01

I cleaned it up a bit by getting the fetched variables assembled first using a chain of promises. This follows a couple style rules that make it more readable (for me anyway)...

Parse.Cloud.beforeSave("Entry", function(request, response) {
    var entry = request.object;
    var contest = request.object.get("contest");

    var fetchedUser, fetchedContest;
    var errorMessage;

    entry.get("user").fetch().then(function(result) {
        fetchedUser = result;
        return contest.fetch();
    }).then(function(result) {
        fetchedContest = result;
        return fetchedContest.get("timeSlot").fetch();
    }).then(function(fetchedTimeSlot) {
        // now we have all the variables we need to determine validity
        var now = new Date();
        var hasSufficientFunds = fetchedUser.get("fundsAvailable") >= fetchedContest.get("entryFee");
        var contestNotStarted = fetchedTimeSlot.get("startDate") >= now;
        if (hasSufficientFunds && contestNotStarted) {
            contest.increment("entriesCount");
            contest.increment("entriesLimit", 0); //have to do this, otherwise entriesLimit is undefined in save callback (?)
            return contest.save();
        } else {
            errorMessage = (hasSufficientFunds)? 'This contest has already started.' : 'Insufficient Funds.';
            return null;
        }
    }).then(function(result) {
        if (!result) {
            response.error(errorMessage);
        } else {
            if (contest.get("entriesCount") > contest.get("entriesLimit")) {
                response.error('The contest is full.');
            } else {
                response.success();
            }
        }
    }, function(error) {
        response.error(error);
    });
});

Note how we never leave a response.anything() dangling, we always make clear what's flowing to the next promise via a return.

Thank you for such a thorough and straight-forward explanation. I now understand promises much better, and this code runs much reliably than how I had it previously. — vikzilla, Jan 28 '16 at 02:34

Cleaning up Spaghetti Code w/ Promises

2 Answers2