5
\$\begingroup\$

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

  var promise = Preloader.preload('media');
  promise.then(
    function(){
      $rootScope.preload.imagesLoaded = true;
    },
    function(){},
    function(currentStatus){
      $rootScope.preload.currentImg = currentStatus.currentImg;
      $rootScope.preload.totalImg = currentStatus.totalImg;
    }
  );
\$\endgroup\$
  • 2
    \$\begingroup\$ I'm really trying hard not to be "that guy", but I can't help myself: "data" and "media" are plural forms already, so "datas" and "medias" isn't quite right. (The singular forms are "datum" and "medium", by the way.) \$\endgroup\$ – Flambino Mar 20 '14 at 11:11
  • 1
    \$\begingroup\$ Too late, you're that guy ;) \$\endgroup\$ – konijn Mar 20 '14 at 17:28
  • \$\begingroup\$ Ahah you are totally right, I updated my question :) \$\endgroup\$ – Tib Mar 20 '14 at 21:01
6
\$\begingroup\$

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' ?

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.