AngularJS DRY controller structure

Question

The code below represents a situation where the same code pattern repeats in every controller which handles data from the server. After a long research and irc talk at #angularjs I still cannot figure how to abstract that code, inline comments explain the situations:

myApp.controller("TodoCtrl", function($scope, Restangular,
                                      CalendarService, $filter){
    var all_todos = [];
    $scope.todos = [];
    Restangular.all("vtodo/").getList().then(function(data){
        all_todos = data;
        $scope.todos = $filter("calendaractive")(all_todos);
    });
    //I can see myself repeating this line in every 
    //controller dealing with data which somehow relates
    //and is possibly filtered by CalendarService:
    $scope.activeData = CalendarService.activeData;
    //also this line, which triggers refiltering when
    //CalendarService is repeating within other controllers
    $scope.$watch("activeData", function(){
        $scope.todos = $filter("calendaractive")(all_todos);
    }, true);

});

//example. another controller, different data, same relation with calendar?
myApp.controller("DiaryCtrl", function($scope, Restangular,
                                       CalendarService, $filter){
    //this all_object and object seems repetitive,
    //isn't there another way to do it? so I can keep it DRY?
    var all_todos = [];
    $scope.todos = [];
    Restangular.all("diary/").getList().then(function(data){
        all_diaries = data;
        $scope.diaries = $filter("calendaractive")(all_diaries);
    });
    $scope.activeData = CalendarService.activeData;
    $scope.$watch("activeData", function(){
        $scope.todos = $filter("calendaractive")(all_diaries);
    }, true);
});

Answer 1

It looks like the code in the 2 controllers is identical except for the path to which the API call is made: "vtodo/" or "diary/".

One way to achieve something closer to DRY-ness is to pass the API path as an option to the controller as an attribute. So, assuming we call the controller ApiController, this can be used as

<div ng-controller="ApiController" api-controller-path="vtodo/">
  <!-- Todo template -->
</div> 

and

<div ng-controller="ApiController" api-controller-path="diary/">
  <!-- Diary template -->
</div> 

Which is then accessible in the controller by the injected $attrs parameter:

myApp.controller("ApiController", function($scope, $attrs, Restangular, CalendarService, $filter) {
  // "vtodo/" or "diary/"
  var apiPath = $attrs.apiControllerPath;

As a caution I would beware of over-architecting, not everything needs to be factored out, and there is an argument that you are just following a design pattern rather than copy+pasting code. However, I have used the above method of passing options to a controller for a similar situation myself.

Answer 2

If you are worried about making multiple calls to the same resource CalendarService, I'd recommend finding a way to cache the result, such as defining a variable in that service or using Angular's $cacheFactory.

Otherwise, I don't see anything wrong with your patterns.

Answer 3

DRY should be followed purposefully, not zealously. Your code is fine, the controllers are doing what they are supposed to be doing: connecting different pieces of the app. That said, you can easily combine repeated code in a factory method that returns a function reference.

For example,

myApp.factory('calendarScopeDecorator', function(CalendarService, Restangular, $filter) {
  return function($scope, section) {
    $scope.todos = [];
    $scope.activeData = CalendarService.activeData;
    Restangular.all(section+"/").getList().then(function(data){
      $scope.all_data = data;
      $scope.filtered_data = $filter("calendaractive")(data);
    });
    $scope.$watch("activeData", function(){
      $scope.todos = $filter("calendaractive")($scope.all_data);
    }, true);
  }
});

And then incorporate this into your controller:

myApp.controller("DiaryCtrl", function($scope, calendarScopeDecorator){
  calendarScopeDecorator($scope, 'diary');
});

Answer 4

I wouldn't do this kind of thing with a watcher and local reference like in this controller. Instead, I would use $on() / $emit() to establish a pub-sub pattern from the service out to the controllers that care about its updates. This is an under-used pattern IMO that provides a more "DRY" mechanism. It's also extremely efficient - often more so than a watcher, because it doesn't need to run a digest to know something has changed. In network-based services you almost always know this for certain, and you don't need to go from knowing it for certain to implying it in other locations. This would let you avoid the cost of Angular's deep inspection of objects:

$rootScope.$on('calendarDiariesUpdated', function() {
    // Update your $scope.todos here.
}, true);

In your service:

// When you have a situation where you know the data has been updated:
$rootScope.$emit('calendarDiariesUpdated');

Note that emit/on are more efficient than using broadcast, which will go through all nested scopes. You can also pass data from the service to listening controllers this way.

This is a really important technique that does a few things:

1. You no longer need to take a local reference to activeData, since you aren't actually using it (it's DRY).

2. This is more efficient in most/many cases than a watcher. Angular doesn't need to work out that you need to be told of an update - you know you do. This is also kind of a DRY principle - why use a framework tool to do something you don't actually need? It's an extra step to put the data somewhere and then wait for Angular to digest it and say "whoah, you need to know about this."

3. You may even be able to reduce your injections. There's no need to take CalendarService because that service can pass a reference to the array right in its notification. That's nice because you don't need to refactor this later if you change the storage model within the service (one of the things DRY advocates also advocate is abstracting these things).

You do need to take $rootScope so you can register the watcher, but there's nothing in pub-sub concepts that violate DRY. It's very common and accepted (and most important: it performs very well). This isn't the same thing as a raw global variable, which is what scares people off from using $rootScope in the first place (and often rightly so).

If you want to be "Super DRY" you can re-factor the call to $filter into a single method that does the filtering, and call it both from your REST promise resolution and from the calendar-update notification. That actually adds a few lines of code... but doesn't REPEAT any. :) That's probably a good idea in principle since that particular line is something you're likely to maintain (it takes that static "calendaractive" parameter...)