HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Angular promise in service

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
serviceangularpromise

Problem

I'm new with Angular promise and I would like to know how to improve my code.

I have a service which preload media or data by calling an API.
The media API return an array of urls to preload.

'use strict';

angular.module('myApp').service('PreloaderService', ['$http', '$q', 'localStorageService', function PreloaderService($http, $q, localStorageService) {
  this.preload = function(key){
    var current = 0;
    var deferred = $q.defer();
    switch (key) {
      case 'media':
        // Simplified, in fact it call  a config service to get baseUrl
        var url = 'http://myapi/media';

        $http.get(url, {cache: true}).then(function(data) {
          var data = data.data;
          preloadImage(data, localStorageService.get('cachetoken'));
        }, function(data) {
          deferred.reject(data);
        });
        break;
      case 'data':
        var url = 'http://myapi/data';
        $http.get(url, {cache: true}).then(function(data) {
          var data = data.data;
          localStorageService.add('data', data);
          deferred.resolve(data);
        }, function(data) {
          deferred.reject(data);
        });
        break;
    }

    function preloadImage(arrayOfImages, cachetoken) {
      var promises = [];
      var totalImg = arrayOfImages.length;
      for (var i = 0; i < totalImg; i++) {
        (function(url, promise) {
          var img = new Image();
          img.onload = function() {
            var currentStatus = {
              currentImg: ++current,
              totalImg: totalImg
            };
            deferred.notify(currentStatus);
            promise.resolve();
          };
          img.src = url + '&tok=' + cachetoken;
        })(arrayOfImages[i], promises[i] = $.Deferred());
      }

      $.when.apply($, promises).done(function() {
        deferred.resolve('RESOLVED');
      }).fail(function(data) {
        deferred.reject(data);
      });
    }

    return deferred.promise;
  };
}]);


And I call it with

`

Solution

This took a while to parse, it's probably me, I found few flaws with the code, except for a distinct lack of comments.

-
I would have made url in preload an optional, 2nd parameter, I would probably want to call preload a few times with different URL's ( for 'data' )

-
I think using a switch for media and data is overkill, I would have done a simple if statement.

-
Since you have a number of times function(data) {deferred.reject(data);});, I would have assigned this to an actual function.

-
{cache: true}, for data seem unfortunately hardcoded, consider this as well as optional parameter ?

-
You define current on top, but totalImg in preloadImage, pick one spot and put both vars there

-
Sometimes you postfix with Img, sometimes with Images. I would stick everywhere to Images, it is so much more readable

-
Except for the angular.module('myApp').service( line, you have succinct lines of code, nice to read

-
No need to store a shortcut to data.data in var data = data.data;preloadImage(data, localStorageService.get('cachetoken'));

-
I would not have gone down the road of managing an array of promises, I probably would have done a simple if (current===totalImg) deferred.resolve('RESOLVED');

-
'RESOLVED' seems kind of pointless to provide as a parameter to resolve, we know it's resolved ;) Perhaps totalImg + ' images loaded' ?

Context

StackExchange Code Review Q#44840, answer score: 6

Revisions (0)

No revisions yet.