0

when I define the factory method like this

dataFactory.all = function() {
  return 'test01';
}

and call it in controller like this

console.log(Data.all());

I can get test01 printed. However, when I add some logic in factory all() method like this

  dataFactory.all = function() {

    $http
      .get('/api/hey')
      .success(function(data) {

        $http
          .get('/api/hi')
          .success(function(data) {

            return 'test'; // 'test' can not be printed, and console show 'undefined'
          });
      });
     //return 'test01'; //can be printed;
  };

then the 'test' can not be printed via the controller. Because I put the return statement in the callback?

Please let me know what I am doing wrong?

Thanks.

leonsPAPA
  • 657
  • 2
  • 13
  • 29

5 Answers5

1

you are doing some asynchronous calls in your code, please visit this question to get more details and information How do I return the response from an asynchronous call?

Community
  • 1
  • 1
atfornes
  • 468
  • 5
  • 21
1

Yes, exactly as you pointed out, you put the return in the callback function. the all() function has no return statement, and by default all functions will return undefined.

The issue you're encountering is that you're now dealing with an asynchronous function that will return data at an unknown time in the future (as opposed to synchronous functions, which return data immediately to be available to the next line of code).

In this case, you're using $http methods, and $http.get() will return a promise. https://docs.angularjs.org/api/ng/service/$q

Check out the examples on the $http documentation. Specifically, look at what they do with the then() functions: https://docs.angularjs.org/api/ng/service/$http

HankScorpio
  • 3,612
  • 15
  • 27
1

I don't think its because you've built it incorrectly, but it looks as though your factory ($http) method is running asynchronously. Try returning a promise like so:

datafactory.all = function() {

   return new Promise(resolve, reject){
       // Your logic here...

     $http
        .get('/api/hey')
        .success(function(data) {
            resolve(data);
        })
        .fail(function(err){
           reject(err);
        });
     }

};

Then you can consume your factory method like so:

datafactory.all().then(function(result){
   // Success!
}, function(err){
   // Error!
});
Matthew Merryfull
  • 1,466
  • 18
  • 32
1

Take a look at angular's $q (docs). With $q you can create a deferred object and return it's promise from your factory method.

That should allow you to retrieve the resulting data from your $http call in a then callback since your factory will be returning a promise:

dataFactory.all = function() {
  // create deferred object
  var deferred = $q.defer();

  $http.get('/api/hey')
  .success(function(data) {

    $http.get('/api/hi') 
    .success(function(data) {

      // resolve the promise with your http request result, this value will 
      // be the result passed to  your factory method's 
      // `then` callback
      deferred.resolve(data);
    }
  }

  return deferred.promise;
}

update

@HankScorpio is right, this could be simplified greatly by returning the $http.get function calls because they return promises themselves:

dataFactory.all = function() {
  return $http.get('/api/hey').success(function(response) {
    return $http.get('/api/hi');
  });
}

Working update is here: http://plnkr.co/edit/wtMnOjjUnKV5x8CQrbIm?p=preview

paul trone
  • 702
  • 5
  • 10
  • 1
    It's not necessary to use `$q.defer()` Simply returning the result of `$http.get()` (in both places where $http.get() is used) would be enough to get this function working. $q should chain the promises together. – HankScorpio Feb 04 '16 at 22:16
  • yeah, got you, I think so as well@HankScorpio – leonsPAPA Feb 04 '16 at 22:20
  • Wondering what's the main drawback to chain the promise?low efficiency or just making the logic complicate, thanks@HankScorpio – leonsPAPA Feb 04 '16 at 22:30
  • @HankScorpio good call on sticking with `$http.get`'s promises, updated answer. – paul trone Feb 04 '16 at 22:45
  • There isn't really a downside in this case. It's not less efficient. If anything it's more efficient because there are fewer promises to create overall and less code to write. It might be harder to understand for someone unfamiliar with promises, but it's effectively the same. – HankScorpio Feb 04 '16 at 22:49
  • An example of where the $q.defer() approach might be better would be if you wanted to inspect the data that you receive and then either `resolve()` or `reject()` the promise based on that data. You wouldn't have that freedom without adding that extra promise. – HankScorpio Feb 04 '16 at 22:51
  • Thanks alot, yeah, I am still using the promise solution instead of just add nested 'return' statement, because I need to add some error/exception handling@HankScorpio – leonsPAPA Feb 06 '16 at 20:31
1

Because you are not returning any value in the scope of the all method and also you need to pay attention that $http return a promise.

So.. your method could be somemething like this:

dataFactory.all = function(jobname) {
  return $http.get('/api/jobs/' + jobname + '/realtimeData').success(function(realtime) {
    return $http.get('/api/jobs/' + jobname + '/nonliveData').success(function(nonlive) {
      return 'test';
    });
  });
};

Or.. to void nested promises you could refactor a bit your code using Flattened Promise Chains :

dataFactory.all = function(jobname) {
  return $http.get('/api/jobs/' + jobname + '/realtimeData').then(function(realtime) {
    // do something with realtime
    return $http.get('/api/jobs/' + jobname + '/nonliveData');
  }).then(function(nonlive){
    return 'test';
  });
};

And because your method all now return a promise, you need to:

dataFactory.all(jobname).then(function(nonlive){
  console.log(nonlive); // should return test!
});
manzapanza
  • 6,087
  • 4
  • 39
  • 48
  • yeah, you are also right, it helps alot. but I see another answer first, so i can only give you one vote, thanks a lot@manzapanza – leonsPAPA Feb 04 '16 at 22:18