Writing Clean Code With Nested Promises

Question

I'm writing an app that talks to Apple to verifyReceipts. They have both a sandbox and production url that you can post to.

When communicating with Apple, if you receive a 21007 status, it means you were posting to the production url, when you should be posting to the sandbox one.

So I wrote some code to facilitate the retry logic. Here's a simplified version of my code:

var request = require('request')
  , Q = require('q')
  ;

var postToService = function(data, url) {
  var deferred = Q.defer();
  var options = {
    data: data,
    url: url
  };

  request.post(options, function(err, response, body) {
    if (err) { 
      deferred.reject(err);
    } else if (hasErrors(response)) {
      deferred.reject(response);
    } else {
      deferred.resolve(body);
    }
  });

  return deferred.promise;
};

exports.verify = function(data) {
  var deferred = Q.defer();

  postToService(data, "https://production-url.com")
    .then(function(body) {
      deferred.resolve(body);
    })
    .fail(function(err) {
      if (err.code === 21007) {
        postToService(data, "https://sandbox-url.com")
          .then(function(body){
            deferred.resolve(body);
          })
          .fail(function(err) {
            deferred.reject(err);
          });
      } else {
        deferred.reject(err);
      }

    });

  return deferred.promise;
};

The retry portion in the verify function is pretty ugly and difficult to read with the nested promises. Is there a better way of doing this?

Answer 1

In case running on an environment that supports the async function, and you are not satisfied by the method of composing promises (which actually is a very clean way to avoid nested promises) you could use something like the below - which is influenced by the Rules and Chain of Responsibility design patterns:

Definitions

• Since we need to use nested promises for the retry logic, we will refer to these promises as chain of retry promises.
• We make the assumption that the environment we're running supports the async function.
• Please note that on the below implementation, some implementation details have been omitted (such as the actual implementation of the POST requests), in order to keep the answer focused on the design.

Implementation

1. Firstly, we could define a generic abstract class for the promises belonging to the chain of retry promises. That is:

export abstract class BasePromise<T> {
    protected url: string;
    protected timeout: number;
    protected data: any;

    public constructor(url: string, timeout: number, data: any) {
        this.url = url;
        this.timeout = timeout;
        this.data = data;
    }

    abstract isMatch(response: any): boolean;

    public async execute(chainedResponse: any):Promise<string> {
      const timeoutPromise: Promise<any> = new Promise((resolve, reject) => setTimeout(reject, timeout, 'Request to {url} timed out'));
      const postToUrlPromiseResult = await Promise.race([timeoutPromise, postToService(data, url)]);

      return postToUrlPromiseResult;
    }
}

2. For the retry logic, we're gonna need 2 promises in our chain. The first one would be the PostToProductionUrlPromise whilst the second one would be the PostToSandboxUrlPromise.
   
   Below is a concrete implementation of the interface we defined earlier, that would allow us to create these 2 promises with the appropriate parameters through constructor dependency injection of the interesting variables (for the sake of it, added support for timeout as well):

const ERROR_POST_TO_SANDBOX_INSTEAD = 21007;

export class PostToProductionUrlPromise extends BasePromise<string> {

    public isMatch(chainedResponse: any): boolean {
        return true;
    }
}

export class PostToSandboxUrlPromise extends BasePromise<string> {

    public isMatch(chainedResponse: any): boolean {
        switch (chainedResponse.code) {
            case ERROR_POST_TO_SANDBOX_INSTEAD:
                return true;
            default:
                return false;
        }
    }
}

3. On this step, we will be defining an Executor class which will be responsible for looping through an array of pre-defined promises and execute them. It would be fine if this code was in a client class as well:

const definedPromises: BasePromise<any>[] = [new PostToProductionUrlPromise(productionUrl, productionTimeout, dataToPost), new PostToSandboxUrlPromise(sandboxUrl, sandboxTimeout, dataToPost)];
const ERROR_POST_TO_SANDBOX_INSTEAD = 21007;
const ERROR_CODES_THAT_ALLOW_FURTHER_EXECUTION = [ERROR_POST_TO_SANDBOX_INSTEAD];

export class ChainOfRetryPromisesExecutor {

    public async executeChain(): Promise<any> {
        let promiseExecutionResult: any;

        for (let i = 0; i < chainOfPromises.length; i++) {
            if (chainOfPromises[i].isMatch(promiseExecutionResult)) {

                promiseExecutionResult = await chainOfPromises[i].execute(promiseExecutionResult)
                    .catch((error: any) => {
                        switch (error.code) {
                            case const ERROR_CODES_THAT_ALLOW_FURTHER_EXECUTION.includes(error.code):
                                return Promise.resolve(error);
                            default:
                                return Promise.reject(error);
                        }
                    });
            }
        }

        return promiseExecutionResult;
    }
}

The Executor class will continue to the PostToSandboxUrlPromise only in case the execution result of the previous promise has returned a code of 21007.

Similarly, you could handle other responses and pass them down the chain for different scenarios.

This implementation supports a bunch of best practices and design pronciples and could potentially provide the needed freedom to expand the handling of the responses without modifying existing code - OCP.

This might be going too far for just a "retry" functionality. However, since this post is trending on google under clean code and nested promises thought that it might help someone looking for alternatives.

Cheers,
Ioannis

Answer 2

Stuart's answer is right, the point was to chain promises. and I would like to clarify that it is not needed to use Q.defer just for wrapping. it's even considered an anti-pattern. See reasons here The Deferred anti-pattern

var request = require('request')
    , Q = require('q');

var PRODUCTION_URL = "https://production-url.com",
var SANDBOX_URL    = "https://sandbox-url.com",


export.verify = function() {

  return postToProduction(data)
         .fail( function(error) {
             if (error.code === 21007 ) return postToSanbox(data);
             throw error;
         });
}

function postToProduction(data) {
    return postToService(data, PRODUCTION_URL);
}

function postToSandbox(data) {
    return postToService(data, SANDBOX_URL);
}

function postToService(data, url) {
   var deferred = Q.defer();

   var options = {
      data: data,
      url: url
   };

  request.post(options, function(err, response, body) {
    if (err) return deferred.reject(err);
    if (hasErrors(response)) return deferred.reject(response);

    deferred.resolve(body);    
  });

  return deferred.promise;   
}

Answer 3

You can re-throw an error in the rejection handler to continue rejecting the promise, or you can return a new promise to replace the rejection.

exports.verify = function(data) {
  return postToService(data, "https://production-url.com")
    .fail(function(err) {
      if (err.code === 21007) {
        return postToService(data, "https://sandbox-url.com")
      } else {
        throw err
      }
    });
};

Answer 4

You might consider something like the following. I think judicious use of whitespace can help readability. You'll probably want to find a reasonable style standard that your team feels good about and stick with it!

exports.verify = function(data) {
  var deferred = Q.defer();

  postToService(data, "https://production-url.com")

    .then(deferred.resolve, function(err) {

      if (err.code === 21007) {

        postToService(data, "https://sandbox-url.com")

          .then(deferred.resolve, deferred.reject);

      } else { deferred.reject(err); }

    });

 return deferred.promise;
};

Answer 5

Here are a couple of possibilities. Because this question has an element of personal taste to it, you may or may not like what you see!

(Admission - I have not tested this code)

Option 1 - Use a wrapper for resolve and reject. This adds 'noise' in the form of the helper functions, but tidies up the rest.

var resolve = function (deferred, ob) {
  return function () {
    deferred.resolve(ob);
  };
};

var reject = function (deferred, ob) {
  return function () {
    deferred.reject(ob);
  };
};

exports.verify = function(data) {
  var deferred = Q.defer();

  postToService(data, "https://production-url.com")
    .then(resolve(deferred, body))
    .fail(function(err) {
      if (err.code === 21007) {
        postToService(data, "https://sandbox-url.com")
          .then(resolve(deferred, body))
          .fail(reject(deferred, err));
      } else {
        deferred.reject(err);
      }
    });

  return deferred.promise;
};

Option 2 - Use bind. This has the advantage of using existing JS functionality, but you have duplicate references to deferred when creating the callbacks.

exports.verify = function(data) {
  var deferred = Q.defer();

  postToService(data, "https://production-url.com")
    .then(deferred.resolve.bind(deferred, body))
    .fail(function(err) {
      if (err.code === 21007) {
        postToService(data, "https://sandbox-url.com")
          .then(deferred.resolve.bind(deferred, body))
          .fail(deferred.reject.bind(deferred, err));
      } else {
        deferred.reject(err);
      }
    });

  return deferred.promise;
};

Option 3 - Use bind and 'method handles' (minor variation on #2).

exports.verify = function(data) {
  var deferred = Q.defer();
  var resolve = deferred.resolve;
  var reject = deferred.reject;

  postToService(data, "https://production-url.com")
    .then(resolve.bind(deferred, body))
    .fail(function(err) {
      if (err.code === 21007) {
        postToService(data, "https://sandbox-url.com")
          .then(resolve.bind(deferred, body))
          .fail(reject.bind(deferred, err));
      } else {
        deferred.reject(err);
      }
    });

  return deferred.promise;
};

Option 4 - Monkey patch deferred.

function patch(deferred) {
  deferred.resolveFn = function (ob) {
    return function () {
      deferred.resolve(ob);
    };
  };
  deferred.rejectFn = function (ob) {
    return function () {
      deferred.reject(ob);
    };
  };
  return deferred;
}

exports.verify = function(data) {
  var deferred = patch(Q.defer());

  postToService(data, "https://production-url.com")
    .then(deferred.resolveFn(body))
    .fail(function(err) {
      if (err.code === 21007) {
        postToService(data, "https://sandbox-url.com")
          .then(deferred.resolveFn(body))
          .fail(deferred.rejectFn(err));
      } else {
        deferred.reject(err);
      }
    });

  return deferred.promise;
};