AngularJS Todo List - too much controller logic?

Question

I am starting an angular app and I was wondering if I am on the right track with this. I worked on my last angular app when I was just a youngun who didn't understand the important of designing the code properly. I failed so hard at keeping the code clean and maintainable, mostly due to not understanding directives or services.

I am wondering if there is a better structure to my error handling in the below code. Is that too much logic in the controller functions? All I am doing is allowing the adding of items to a list, but not allowing them to be added or removed if the list length is < 5 or > 1 respectively. If there are other problems besides the controller logic, please let me know.

var toDo = angular.module('toDo',[]);

toDo.factory('listEdit', function() {
return {
    add: function(list, item) {
        list.push(item);
        return list;
    },
    remove: function(list, index) {
        list.splice(index, 1);
        return list;
    },
    count: function(list) {
        return list.length;
    }
}
});

//Error handling methods

toDo.controller('main', ['$scope', 'listEdit', function($scope, listEdit) {
$scope.list = [];

$scope.addToList = function() {
    list_length = listEdit.count($scope.list);
    if(list_length < 5) {
        $scope.list = listEdit.add($scope.list, $scope.todo);
    } else {
        alert('Too many list items');
    }
}

$scope.remove = function(index) {
    list_length = listEdit.count($scope.list);
    if(list_length > 1) {
    $scope.list = listEdit.remove($scope.list, index);
    } else {
        alert("You can't delete this last item");
    }
}

}]);

HTML

<body ng-controller="main">
    <input type="text" ng-model="todo">
    <button ng-click="addToList()">Add to List</button>
    <div ng-repeat="todo in list track by $index">
    <span ng-click="remove($index)">{{todo}}</span>
    </div>
</body>

Thomas Sebastian · Accepted Answer · 2015-02-06 09:52:01Z

This is a funny question because

One of the most readable Angular submissions on CR
There is probably too much logic in the controller indeed

Some observations:

It seems the controller knows too much, it should not know that 5 is the maximum amount of items, or that 1 is the minimum amount of items to be kept
It seems silly to pass the list to list every single time to listEdit, it is not DRY.
I know alert in this case is used because it is a prototype, but still..

I would counter propose something where listEdit is aware of the list after you make a connection, listEdit also knows about limits and gives feedback ( in the form of a string ) whenever it does something to the list.

The controller this way is very small, and easy to understand:

//Controller methods
toDo.controller('main', ['$scope', 'listEdit', function($scope, listEdit) {

  $scope.list = [];
  listEdit.linkList( $scope.list ); //<- Magic happens here

  $scope.addToList = function() {
    $scope.feedback = listEdit.add($scope.todo);
  };

  $scope.remove = function(index) {
    $scope.feedback =  listEdit.remove(index);
  };

}]);

listEdit is then a little bigger:

toDo.factory('listEdit', function() {
  var _list = [],
      maxLength = 5,
      minLength = 1;

  return {
    add: function(item) {
      if( _list.length >= maxLength )
        return 'Too many list items';
      _list.push(item);
      return item + ' is added';
    },
    remove: function(index) {
      if( _list.length == minLength )
        return 'You can\'t delete this last item';                
      var item = _list.splice(index, 1)[0];
      return item + ' has been deleted';
    },
    count: function() {
        return internalList.length;
    },
    linkList: function(list){
      _list = list;
    }
  };
});

I consciously chose for immediate return with feedback in case of a problem, then the intended activity and then giving positive feedback. My evil twin whispered this hackish alternative:

    add: function(item) {
      return _list.length >= maxLength ?
        'Too many list items' :
        _list.push(item), item + ' is added';
    }

Also, you will find me railing against people using underscore to indicate that it is private when it is not. In this case, _list is actually private, so that is fine.

In order to show the feedback you would need to add div:

<!DOCTYPE html>
<html ng-app="toDo" >
<head>
<script src="https://ajax.googleapis.com/ajax/libs/angularjs/1.2.14/angular.min.js"></script>
  <meta charset="utf-8">
  <title>JS Bin</title>
</head>
<body ng-controller="main">
    <div>{{feedback}}&nbsp;</div>
    <input type="text" ng-model="todo">
    <button ng-click="addToList()">Add to List</button>
    <div ng-repeat="todo in list track by $index">
    <span ng-click="remove($index)">{{todo}}</span>
    </div>
</body>
</html>

I used the   trick to make sure the elements dont reflow after the first element is added, probably not best practice, HTML is not my forte.

In the end, it was fun to play with this in jsbin: http://jsbin.com/vesomu/2/edit

Also, check this : a simple todo app by Angular pro's.

Thank's a lot konijin. Sorry for the late response, couldn't get into my account. — evwill, Commented Aug 24, 2014 at 15:48

Dmitri Zaitsev · Accepted Answer · 2014-08-06 15:25:23Z

Some remarks and refinements of previous answer but too long to put in the comments.

Instead of

<button ng-click="addToList()">Add to List</button>

I would make it slightly more expressive and re-usable:

<button ng-click="addToList(todo)">Add to List</button>

That is also consistent with remove($index), which I would rename to removeItem($index).

I would name the service ListService instead of ListEdit.
Yet another renaming :) - _maxLength instead of maxLength, to make it consistent with _list.

Also added the counter (note that a function is called on every $digest):

  <div>Total: {{count()}} items!</div>

then in the controller:

$scope.count = listService.count;

linking with the service:

count: function() {
    return _list.length;
}

I could have made a shortcut and put

$scope.count = function(){return $scope.list.length}

which is however not a good practice, as it is the business of controller is to pass things, not to perform things.

There is yet a shorter Angular-magical shortcut :) putting .length right inside HTML:

<div>Total: {{list.length}} items</div>

A big pro is huge reduction in boilerplatte - no more service nor controller methods!

I prefer to chain module methods:

angular.module('App',[])
.controller(...)
.factory(...)

instead of

app = angular.module('App',[])
app.controller(...)
app.factory(...)

The latter creates a global variable that I have to remember and maintain and carefully watch not to misprint.

I would use the new controller as syntax:

<body ng-controller="Main as MC">
    <button ng-click="MC.addToList()">Add to List</button>
    ...
</body>

and then inside controller:

var MC = this;

MC.addToList = function(todo) {
   MC.feedback = listService.add(todo);
};

That way the scope is properly encapsulated and will not clash with other controllers via $scope inheritance (unless that is what I am after). Also I am renaming this to MC (Main Controller), which protects against change of this (e.g. inside event listeners), and make every property look exactly the same inside HTML and the Controller, which makes things very readable.

Last but not least I have changed the way controller's list is linked with one of the service. That is

$scope.list = listService.getList();

instead of

listService.linkList($scope.list);

That way the original list is not in the Controller but the Service, and Controller simply asks for it.

Here the jsbin to enjoy!

Stack Exchange Network

AngularJS Todo List - too much controller logic?

2 Answers 2

Your Answer

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

Hot Network Questions

AngularJS Todo List - too much controller logic?

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

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