Is this a "Deferred Antipattern"?

Question

I'm finding it hard to understand the "deferred antipattern". I think I understand it in principal but I haven't seen a super simple example of what a service, with a differed promise and one with antipattern, so I figured I'd try and make my own but seeing as how I'm not super in the know about it I'd get some clarification first.

I have the below in a factory (SomeFactory):

//url = 'data.json';

return {
    getData: function(){
        var deferred = $q.defer();

        $http.get(destinationFactory.url)
            .then(function (response) {

                if (typeof response.data === 'object') {
                    deferred.resolve(response.data);
                } else {
                    return deferred.reject(response.data);
                }
            })

            .catch(function (error) {
            deferred.reject(error);
        });

        return deferred.promise;
    }

The reason I am checking its an object is just to add a simple layer of validation onto the $http.get()

And below, in my directive:

this.var = SomeFactory.getData()
    .then(function(response) {
        //some variable = response;
    })
    .catch(function(response) {
        //Do error handling here
});

Now to my uderstanding, this is an antipattern. Because the original deferred promise catches the error and simply swallows it. It doesn't return the error so when this "getData" method is called I have do another catch to grab the error.

If this is NOT an antipattern, then can someone explain why both require a "callback" of sorts? When I first started writing this factory/directive I anticipated having to do a deffered promise somewhere, but I didn't anticipate having to .catch() on both sides (aka I was sort of thinking I could get the factory to return the response or the error if I did a SomeFactory.getData()

Yonkers

Answer 1

Is this a “Deferred Antipattern”?

Yes, it is. 'Deferred anti-pattern' happens when a new redundant deferred object is created to be resolved from inside a promise chain. In your case you are using $q to return a promise for something that implicitly returns a promise. You already have a Promise object($http service itself returns a promise), so you just need to return it!

Here's the super simple example of what a service, with a deferred promise and one with antipattern look like,

This is anti-pattern

app.factory("SomeFactory",['$http','$q']){
    return {
        getData: function(){
            var deferred = $q.defer();            
            $http.get(destinationFactory.url)
              .then(function (response) {        
                 deferred.resolve(response.data);
            })
              .catch(function (error) {
                deferred.reject(error);
            });            
            return deferred.promise;
        }
     }
}])

This is what you should do

app.factory("SomeFactory",['$http']){
    return {
        getData: function(){
           //$http itself returns a promise 
            return $http.get(destinationFactory.url);
        }
}

while both of them are consumed in the same way.

this.var = SomeFactory.getData()
    .then(function(response) {
        //some variable = response;
    },function(response) {
        //Do error handling here
});

There's nothing wrong with either examples(atleast syntactically)..but first one is redundant..and not needed!

Hope it helps :)

Pioneer

Hi NLN, Thanks for the reply. So with your example, is there anyway to include the typeof validation or would that be done in the controller after retrieving from the factory? – Yonkers

Hi Aleski. You can perform the typeof validation in .success() function :) – Pioneer

I made an edit without logging in and now it's locked. Basically, .success and .error have been deprecated. You can just use .then now. – Brilliant

@SenHeng: Your edit seems nice(and necessary, sortof), thanks :) – Pioneer

I don't think first one is redundant and not needed. May be you want to manipulate data in your factory and save there a value shared by others controllers... – Clamper

@IsraGab: Yes you're right. There could be situations when we might need to manipulate or save/persist the results from a particular response. If this is needed by every controller, we should better move that code to some other factory/service. There are 2 reasons to do this. 1) Factories/Services are singleton, and we should respect separation of concerns when dealing with them. 2) It'll make our life easier when writing unit tests :) – Pioneer

Are the two examples really the same? It looks like the first "antipattern" example resolves response.data, but the plain $http promise resolves response. Can the two examples really be consumed the same if they resolve to different objects? – Enclasp

Answer 2

I would say that it is the classic deferred anti-pattern because you are creating needless deferred objects. However, you are adding some value to the chain (with your validation). Typically, IMO, the anti-pattern is particularly bad when deferred objects are created for very little or no benefit.

So, the code could be much simpler.

$q promises have a little documented feature of automatically wrapping anything returned inside a promise in a promise (using $q.when). In most cases this means that you shouldn't have to manually create a deferred:

var deferred = $q.defer();

However, that is how the documentation demonstrates how to use promises with $q.

So, you can change your code to this:

return {
    getData: function(){
        return $http.get(destinationFactory.url)
            .then(function (response) {
                if (typeof response.data === 'object') {
                    return response.data;
                } else {
                    throw new Error('Error message here');
                }
            });

            // no need to catch and just re-throw
        });
    }

Messeigneurs

Right ok so because of the addition of my light validation, I should leave the "callback"-y then part because I am then doing something with it BEFORE returning it? How common is it to do some work inside the promise before consuming it? I like the look more of NLN's answer but he had to drop my validation... – Yonkers

It is very common to read data from a promise chain, do some logic and then return data for further processing. That is what makes it a chain :). Finally, you reach the end and bind the data to $scope or where ever. – Messeigneurs

So really the issue here is with getting $q involved needlessly? – Yonkers

Yes, you can imagine that you have a promise chain that is several layers. Creating a deferred for every link in the chain would be cumbersome, and not necessary. – Messeigneurs

Answer 3

Using the $q constructor is a deferred anti-pattern

ANTI-PATTERN

vm.download = function() {
  var url = "https://www.w3.org/WAI/ER/tests/xhtml/testfiles/resources/pdf/dummy.pdf";    
  return $q(function(resolve, reject) {    
    var req = {
      method: 'POST',
      url: url,
      responseType: 'arraybuffer'
    };   
    $http(req).then(function(response) {
      resolve(response.data);
    }, function(error) {
      reject(error);
    });
  });
}

CORRECT

vm.download = function() {
    var url = "https://www.w3.org/WAI/ER/tests/xhtml/testfiles/resources/pdf/dummy.pdf";    
    var req = {
      method: 'POST',
      url: url,
      responseType: 'arraybuffer'
    };   
    return $http(req).then(function(response) {
        return response.data;
    });
}

The $http service already returns a promise. Using the $q constructor is unnecessary and error prone.

Dervish