current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite

I'm not sure whether my code looks good or not:

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

app.run(function(editableOptions) {
  editableOptions.theme = 'bs3';
});

app.controller('TasksCtrl', [
  '$scope', 'Task', function($scope, Task) {

    $scope.user = gon.current_user

    $scope.updateTitle = function(data, task) {
      Task.update({
        user_id: $scope.user.id,
        id: task.id,
        title: data
      });
    };

    $scope.updatePriority = function(data, task){
      Task.update({
        user_id: $scope.user.id,
        id: task.id,
        priority: data
      })
    };

    $scope.updateDueDate = function(task) {
      var autoclose = this;
      $('ul.tasks form input').datepicker({
        dateFormat: 'yy-mm-dd',
        onSelect: function(date, instance) {
          task.due_date = date;
          Task.update({
            user_id: $scope.user.id,
            id: task.id
          }, {
            due_date: date
          });
          autoclose.$editable.scope.$form.$cancel();
        }
      });
    };

    $scope.tasks = Task.query({
      user_id: $scope.user.id,
      status: 'incompleted'
    });

    $scope.completedTasks = Task.query({
      user_id: $scope.user.id,
      status: 'completed'
    });

    $scope.addNewTask = function() {
      var task = Task.save({user_id: $scope.user.id}, ($scope.newText));
      $scope.tasks.push(task);
      $scope.newText = {};
    };

    $scope.deleteTask = function(task){
      if (confirm('Are you sure')) {
        var index = $scope.tasks.indexOf(task);
        Task.delete({ user_id: $scope.user.id, id: task.id },
          function(success){
            $scope.tasks.splice(index, 1);
        });
      }
    };

    $scope.complete = function(task) {
      Task.update({
        user_id: $scope.user.id,
        id: task.id,
        task: {
          completed: true
        }
      }, function() {
        var index;
        index = $scope.tasks.indexOf(task);
        $scope.tasks.splice(index, 1);
        $scope.completedTasks.push(task);
      });
    };

    $scope.restore = function(task) {
      Task.update({
        user_id: $scope.user.id,
        id: task.id,
        task: {
          completed: false
        }
      }, function() {
        var index;
        index = $scope.completedTasks.indexOf(task);
        $scope.completedTasks.splice(index, 1);
        $scope.tasks.push(task);
      });
    };

    $scope.sort = function(property) {
      var  arrow = $("#" + property);
      var direction;
      $('.sort .glyphicon');
           arrow.addClass('incompleted');
      if (arrow.hasClass('glyphicon-arrow-down')) {
        arrow.removeClass('glyphicon-arrow-down');
        arrow.addClass('glyphicon-arrow-up');
        direction = 'desc';
      } else {
        arrow.addClass('glyphicon-arrow-down');
        arrow.removeClass('glyphicon-arrow-up');
        direction = 'asc';
      }
      $scope.tasks = Task.query({
        user_id: $scope.user.id,
        status: 'incompleted',
        order_by: property,
        direction: direction
      });
    };

  }
]);
share|improve this question
up vote 8 down vote accepted 
$scope.user = gon.current_user

What's gon? I assume it's your authentication/session object with all the things the app needs before running. Best if you put it inside a service or a value so that you can easily use dependency injection with it. That way, your controllers are uniform and only depend on stuff that comes from dependencies, not from extraterrestrial entities like globals.

Task.delete({ user_id: $scope.user.id, id: task.id },function(success){
  $scope.tasks.splice(index, 1);
});

Suggesting you use promises instead of callbacks. They look about the same for simple cases (just a callback passed to then). But promises allow you more control of the data flow. It allows you to modify values, chain successive async operations, wait for multiple parallel promises etc.

var  arrow = $("#" + property);
var direction;
$('.sort .glyphicon');
arrow.addClass('incompleted');
if (arrow.hasClass('glyphicon-arrow-down')) {
  arrow.removeClass('glyphicon-arrow-down');
  arrow.addClass('glyphicon-arrow-up');
  direction = 'desc';
} else {
  arrow.addClass('glyphicon-arrow-down');
  arrow.removeClass('glyphicon-arrow-up');
  direction = 'asc';
}

In frameworks like Angular, you only worry about manipulating the data. Normally, you don't do DOM manipulation in Angular anymore except for cases where you have to, like appending your datetime plugin. You could delegate this down as template logic using a scope variable and ng-class:

$scope.isDescending = false; // ascending by default
$scope.sort = function(){
  $scope.isDescending = !$scope.isDescending; // toggle
  // derrive direction for Task.query
  var direction = ['asc', 'desc'][+$scope.isDescending];
}

<div ng-class="{
  'glyphicon-arrow-down': isDescending,
  'glyphicon-arrow-up': !isDescending
}"></div>
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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