Bookmark application AngularJS controller complexity

I am working on a personal project for managing one's Pinboard bookmarks. Since this is already quite a big project, I am not sure if my question fits the guidelines. My main concern is with the complexity of one of my controllers. I have the feeling I should cut it into smaller controllers or directives, but I am not sure how I could best achieve this.

My project is here and my main concern is the main.js controller in App/scripts/controllers. Because the project already is quite big and depends on numerous other packages and resources, I do not know how to provide a runnable version easily. However, the project should install / run / test / build when cloned from GH.



/**
* @ngdoc function
* @name pinboredWebkitApp.controller:MainCtrl
* @description
* # MainCtrl
* Controller of the pinboredWebkitApp
*/
angular.module('pinboredWebkitApp')
.controller('MainCtrl', function ($scope, $location, $filter, $modal, $q, $splash, $timeout,
Pinboardservice, Usersessionservice, Appstatusservice, Utilservice, Modalservice,
fulltextFilter, tagsFilter) {

// if not authenticated, redirect to login page
if (Usersessionservice.isAuthenticated() === false) {
$location.path('/login');
return;
}

// if logged off, redirect to login page as well
$scope.$on('user:authenticated', function() { // args: event, data
if(Usersessionservice.authenticated === false) {
$location.path('/login');
return;
}
});

// page model
$scope.data = {
loadType : 'recent',
isLoading : true,
activePage : 3,
items: [],
filteredList : [],
selectedItems : []
};

$scope.paging = {
numPageButtons : 10,
current : 1,
total : 0
};

$scope.filter = {
textFilterDelay : 750,
textFilterTimeout : null,
text : '',
tags : []
};

$scope.multiAction = {
show : false,
selectedAction : '',
dangerousAction : false,

Interesting,

the sheer volume of your controller and the style reminded me of the anti-pattern caled The magic servlet from the Bitter Java book. From an MVC perspective you have to extract all bookmark data related code and create a BookMarks singleton class that manages just bookmarks. Then your controller class will just respond to events which change the model which then possibly changes the UI ( UI related functions should probably go into their own class as well).

Other than that, your code passes JSHint pretty cleanly, except for snippets like this:

if($scope.data.selectedItems.length > 0) {
    for(var i=0; i<$scope.data.selectedItems.length; i++) {
      // following is inside anonymous function closure because of loop iterator scope problem
      (function(i, newTagName){
        // set initial status to checking
        $scope.data.selectedItems[i].data.tags += ' ' + newTagName;
        // update bookmark
        Pinboardservice.updateBookmark($scope.data.selectedItems[i])
        .then(function(result) {
          // var updatedBmHash = $scope.data.selectedItems[i]['data']['hash'];
          var updatedBmHash = $scope.data.selectedItems[i].data.hash;
          updated++;
          if(result.result_code === 'done') {
            Appstatusservice.updateStatus('updated bookmark: ' + updatedBmHash + '.', updated, total);
          } else {
            Appstatusservice.updateStatus('Failed: ' + result.result_code + '.', updated, total, 'danger');
          }
        }, function(reason) {
          Appstatusservice.updateStatus('Failed: ' + reason, updated, total, 'danger');
        });
      })(i, newTagName);
    }
  }
};

Creating a closure in this case is indeed acceptabel, but dont create the function in a loop. That is just needless memory wastage, declare the function up front (with a name!) and call that function. Ideally you would call the function with $scope.data.selectedItems[i] instead of i, so that your code will look cleaner.

function addTag( item, tag ){
  //Set initial status to checking
  item.data.tags += ' ' + tag;
  //Update bookmark
  Pinboardservice.updateBookmark(item)
  .then(function(result) {
    var updatedBmHash = item.data.hash;
    updated++;
    if(result.result_code === 'done') {
      Appstatusservice.updateStatus('updated bookmark: ' + updatedBmHash + '.', updated, total);
    } else {
      Appstatusservice.updateStatus('Failed: ' + result.result_code + '.', updated, total, 'danger');
    }
  }, function(reason) {
    Appstatusservice.updateStatus('Failed: ' + reason, updated, total, 'danger');
  });
}

Finally, you could clean up the code, there are a number of commented out statements and console.log and console.info calls you should remove.

StackExchange Code Review Q#63466, answer score: 2