Angular using defer and services

Question

I have wrote some service in AngularJS that use defer, can you check my code and tell me if I getting it right?

Service (I dont really like name: WatchableTeaser, have you got another?):

.factory('Teaser', function ($resource, DateUtils) {
    return $resource('api/teasers/:id', {}, {
        'query': {method: 'GET', isArray: true},
        'getByLangKey': {method: 'GET', url: 'api/teasers/langKey/:langKey', isArray: true},
        'get': {
            method: 'GET',
            transformResponse: function (data) {
                data = angular.fromJson(data);
                return data;
            }
        },
        'update': {method: 'PUT'}
    });
})
.service('WatchableTeaser', function ($q, $cookies, $filter, Language, Teaser) {
    var lastSeenTeaserVideo = Number($cookies.get('last_seen_teaser_video'));
    var getTeasersByCurrentLang = function () {
        var deferred = $q.defer();
        Language.getCurrent().then(function (current) {
            Teaser.getByLangKey({langKey: current}, function (result) {
                deferred.resolve(result);
            })
        });

        return deferred.promise;
    };

    var teasersPromise = getTeasersByCurrentLang();

    this.saveLastSeenTeaser = function (teaser) {
        var deferred = $q.defer();

        if (angular.isNumber(teaser)) {
            deferred.resolve(teaser);
        } else if (angular.isObject(teaser)) {
            teasersPromise.then(function (teasers) {
                deferred.resolve(teasers.indexOf(teaser));
            });
        } else {
            deferred.resolve(-1);
        }

        deferred.promise.then(function(indexOfTeaser) {
            var expireDate = new Date();
            expireDate.setDate(expireDate.getDate() + 30);
            $cookies.put("last_seen_teaser_video", indexOfTeaser, {
                expires: expireDate
            });

            lastSeenTeaserVideo = indexOfTeaser;
        });
    };

    if (isNaN(lastSeenTeaserVideo)) {
        this.saveLastSeenTeaser();
    }

    this.getTeaserToWatch = function () {
        var deferred = $q.defer();
        var self = this;
        teasersPromise.then(function (teasers) {
            var teaserToWatchIndex = lastSeenTeaserVideo + 1;

            if (teaserToWatchIndex < 0 || teaserToWatchIndex >= teasers.length) {
                self.saveLastSeenTeaser();
                teaserToWatchIndex = 0;
            }

            deferred.resolve(teasers[teaserToWatchIndex]);
        });

        return deferred.promise;
    };
});

Controller:

.controller('MainController', function ($scope, $state, $cookies, Category, WatchableTeaser) {
    $scope.categories = [];
    $scope.categoryName = $state.params.categoryName;
    $scope.teaser = {};

    $scope.loadAll = function () {
        Category.query(function (result) {
            $scope.categories = result;
        });

        WatchableTeaser.getTeaserToWatch().then(function (teaser) {
            $scope.teaser = teaser;
        });
    };
    $scope.loadAll();

    $scope.$on('$stateChangeSuccess', function () {
        $scope.categoryName = $state.params.categoryName;
    });

    $scope.safeApply = function (fn) {
        var phase = this.$root.$$phase;
        if (phase == '$apply' || phase == '$digest') {
            if (fn && (typeof(fn) === 'function')) {
                fn();
            }
        } else {
            this.$apply(fn);
        }
    };

    var player = undefined;
    videojs("teaser_video").ready(function () {
        player = this;

        player.on("ended", function () {
            WatchableTeaser.saveLastSeenTeaser($scope.teaser);

            WatchableTeaser.getTeaserToWatch().then(function (teaser) {
                $scope.safeApply(function () {
                    $scope.thumbnailUrl = teaser.thumbnailUrl;
                })
            });
        });
    });

    $scope.exitFullscreen = function () {
        if (angular.isDefined(player)) {
            player.exitFullscreen();
        }
    };

    $scope.watchAgain = function () {
        if (angular.isDefined(player)) {
            player.currentTime(0);
            player.play();
        }
    };

    $scope.showRecomendedVideo = function () {
        if (angular.isDefined(player)) {
            WatchableTeaser.getTeaserToWatch().then(function (teaser) {
                $scope.teaser = teaser;
                player.src($scope.teaser.videoUrl);
                $scope.watchAgain();
            });
        }
    };
});

The question is about implement and usage WatchableTeaser. Thank for any comment.

Answer 1

.factory('Teaser', function ($resource, DateUtils) {
    return $resource('api/teasers/:id', {}, {
        'query': {method: 'GET', isArray: true},
        'getByLangKey': {method: 'GET', url: 'api/teasers/langKey/:langKey', isArray: true},
        'get': {
            method: 'GET',
            transformResponse: function (data) {
                data = angular.fromJson(data);
                return data;
            }
        },
        'update': {method: 'PUT'}
    });
})

It would be nice if you wrote this consistently. I noticed that there's several that you "one-lined" while the other you expanded.

query is at its default settings which I guess can be omitted.

Also, I'm wondering why get requires special treatment. Is there a reason why it needs the additional transform when the others don't? I believe there's a problem elsewhere which you should address.

I'm not really a fan of services (because internally, Angular uses new and you have to put this). I prefer factories because you're in control of what you return.

---

if (isNaN(lastSeenTeaserVideo)) {
    this.saveLastSeenTeaser();
}

Wondering why is this in the middle of all method definitions. Looking at the service, I'd think initializations come first. But then this is lost somewhere in the middle.

var deferred = $q.defer();
Language.getCurrent().then(function (current) {
  Teaser.getByLangKey({langKey: current}, function (result) {
    deferred.resolve(result);
  })
});
return deferred.promise;

This is a common error for people new to the idea of promises - the using of a pre-created deferred to be resolved when the callbacks finish. However, resources already expose promises (via the $promise property). You can simply return that from a then, and the caller will wait for the getByLangKey. instead.

function getTeasersByCurrentLang(){
  return Language.getCurrent().then(function(current){
    return Teaser.getByLangKey({langKey: current}).$promise;
  });
}

// Calling getTeasersByCurrentLang() will return the promise of
// getByLangKey.

getTeasersByCurrentLang().then(function(result){
  // I got the teaser with the current language
});

Now the beauty of promises is that you can either return a promise (like above) or a value. When you return a value from then, the next attached then will resolve with that value instead.

this.getTeaserToWatch = function(){
  return teasersPromise.then(function(teasers){
    var teaserToWatchIndex = lastSeenTeaserVideo + 1;
    if (teaserToWatchIndex < 0 || teaserToWatchIndex >= teasers.length) {
      self.saveLastSeenTeaser();
      teaserToWatchIndex = 0;
    }

    // The `then` that's attached to a `getTeaserToWatch` call will resolve
    // with this value instead. 
    return teasers[teaserToWatchIndex];
  });
}

---

this.saveLastSeenTeaser = function (teaser) {
    var deferred = $q.defer();

    if (angular.isNumber(teaser)) {
        deferred.resolve(teaser);
    } else if (angular.isObject(teaser)) {
        teasersPromise.then(function (teasers) {
            deferred.resolve(teasers.indexOf(teaser));
        });
    } else {
        deferred.resolve(-1);
    }

Here's one problem waiting to happen - a function that accepts a variable argument. Why not create separate functions for each operation? JavaScript may be dynamic, but doing this makes your code really unpredictable. I suggest you break up this function into one that accepts a number, one that accepts an object, and a third which contains their common routine.

---

var player = undefined;
videojs("teaser_video").ready(function () {
    player = this;

    player.on("ended", function () {
        WatchableTeaser.saveLastSeenTeaser($scope.teaser);

        WatchableTeaser.getTeaserToWatch().then(function (teaser) {
            $scope.safeApply(function () {
                $scope.thumbnailUrl = teaser.thumbnailUrl;
            })
        });
    });
});

Your controller looks fine except for this bit of code. I assume this is a video player library of some sort? Your controller should not be involved in page-related activities. In the code above, the controller is aware of a video player on the page.

You could create a directive which manages that video player. Then you define the data and handlers in the controller scope, which you then hand over to the directive. If that directive says the video has ended, it calls that function. Back in the controller, if the function is called, it does some logic which will then reflect back down the page.

// On the controller, you define a handler
$scope.videoEndHandler = function(){
  // All that logic in the "ended handler"
};

// On the template, you pass the data and assign handlers
<video-player thumbnail-url="thumbnailUrl" on-change="videoEndHandler" />

// Inside directive, you call the handlers (or noop, if none was passed)
video.on('ended', onChange || angular.noop);

The nice thing about this approach is should you decide to remove the video player off the page, your controller won't have dead player code lying around. Also, player specific logic (like the listening of the "ended" event from a player instance) is cohesive, and live only in that one directive.