AngularJS to-do list controller

Question

I am new to Angular. Am I following best practices here? I understand controllers should not do too much. My controller seems to have a lot of logic here. Can I do anything to improve it? It is just a basic ToDo App.

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

app.controller('FirstCtrl', function($scope, todos){
    $scope.todos = todos;

    $scope.todos.submit = function(){
        if ($scope.input == "" || $scope.input == undefined){
            return false;
        }
        else{
            $scope.todos.list.push({
                task:$scope.input,
                completed: false
            });
            $scope.input = "";
        }
    }

    $scope.clear = function(){
         $scope.todos.list = $scope.todos.list.filter(function(todo){
            return !todo.completed;
        });
    }

});

app.factory('todos', function(){
    var todos = {};

    todos.list = [];

    return todos;
})

app.directive('todo', function(){
    return {
        restrict: "E",
        templateUrl:'templates/todos.html',
        controller:'FirstCtrl as First'
    }
})

Answer 1

app.controller('FirstCtrl', function($scope, todos){

You're using the implicit dependency injection method which uses the argument name as the dependency name. If you will be minifying the code (which mangles the name, rendering this approach useless), you either have to use explicit dependency (ie. .controller(name, [dep1, dep2, ... fn]) or use ng-annotate.

Additionally, you should name your controller properly. Normally, what we do is append "Controller", "Service" etc. to the name. In this case, it should be like "ToDoController". Also prevents ambiguities in dependency injection.

if ($scope.input == "" || $scope.input == undefined){
  return false;
}

Angular has built-in validation. Usually, I call submit passing in form's valid property and the data. If the valid property of the form is true, then it is safe to assume that the data that came with it has passed Angular's validation.

$scope.todos.list.push({
  task:$scope.input,
  completed: false
});

This operation must be delegated to the factory, not in the controller. The issue is when you start to share data across different controllers. If you happen to have another section of the UI able to add new todos to the main list, then you'd be duplicating your push logic in another controller.

Delegating this to the factory will need you to expose a function that the controller can call, pass in the data, and the factory update the list accordingly. Since the controller has reference to the same list, changes will appear magically.

restrict: "E",

If you're targetting IE8, then you should be aware that "E" isn't an option. You'd have to use "A" and write attribute directives instead of element name directives.

controller:'FirstCtrl as First'

I would recommend that a directive have its own scope ("isolate scope" as they call it), and that the binding to the controller be explicit. My concept of "directives" are like "custom elements" - portable, self-contained code. One can easily write the following:

<to-do>
  <to-do-list list="todos"></to-do-list>
  <to-do-form onSubmit="addTodo"></to-do-form>
</to-do>

Where:

• <to-do> is a wrapper, a directive with a transclude (allows inner html).
• <to-do-list> is a directive that renders the to-do list, given an array to its list property.
• <to-do-form> is a directive that renders an input, which calls a function assigned to onSubmit.

The controller will then look like this:

app.controller('TodoController', function($scope, ToDoFactory){

  // Get a reference to the ToDos and expose it to the template
  $scope.todos = ToDoFactory.getToDos();

  // Expose a way to call a function that adds todos
  $scope.addTodo = function(toDo, isValid){
    if(!isValid) return;
    ToDoFactory.add(toDo);
  }
});

The factory:

app.factory('ToDoFactory', function(){
  var todos = [];

  return {
    add: function(todo){
      todos.push(todo);
    },
    getToDos: function(){
      return todos;
    }
  }
});

The list be like:

app.directive('toDoList', function(){
  return {
    restrict: 'E',
    template: '<ul ng-repeat="toDo in list"><li>{{ todo }}</li></ul>',
    scope: { list: '=' }
  }
});

The input be like:

app.directive('toDoForm', function(){
  return {
    restrict: 'E',
    template: '<form name="toDoForm"><input type="text" ng-model="todo" ...validations...><button type="submit" ng-click="onSubmit(todo, toDoForm.$valid)">Add</button></form>',
    scope: { onSubmit: '=' }
  }
});

Also, directives have a link function which can act as your setup area, much like a controller's function. This is helpful especially when setting up watches, data manipulations etc.