Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

I have an angular app using a restful API which works, but I know is structured poorly.

I'm using a projectService (factory) to do all interfacing with the REST back-end, for both of the backend Project and Task models. The ProjectController and TaskController both get the projectService (factory), and TaskController updates based on $scope.$watch.

REST API

> GET|HEAD api/users/{users}/projects    
POST api/projects                      
PUT api/projects/{projects}            
PATCH api/projects/{projects}          
DELETE api/projects/{projects}         
GET|HEAD api/projects/{projects}/tasks 
POST api/tasks                         
PUT api/tasks/{tasks}                  
PATCH api/tasks/{tasks}                
DELETE api/tasks/{tasks}

app.js

angular.module('todoApp', ['ui.sortable', 'ngRoute', 'ngResource'], function($interpolateProvider) {
    $interpolateProvider.startSymbol('<%');
    $interpolateProvider.endSymbol('%>');
})
.factory('projectService', function($http) {

    return {
        projectID: null,
        getUserProjects: function(id) {
            return $http.get('/api/users/' + id + '/projects');
        },
        getProjectTasks: function(id) {
            var thisID = id || this.projectID;
            return $http.get('/api/projects/' + thisID + '/tasks');
        },
        setProjectId: function(id) {
            this.projectID = id;
        },
        submitProject: function(projectData) {
            console.log(projectData);
            return $http({
                method: 'POST',
                url: '/api/projects',
                headers: { 'Content-Type' : 'application/x-www-form-urlencoded' },
                params: projectData
            });
        },
        deleteProject: function(id) {
            console.log(id);
            return $http.delete('/api/projects/' + id);
        },
        updateProjectTasks: function(tasks) {
            return $http({
                method: 'PUT',
                url: '/api/projects/' + this.projectID,
                headers: { 'Content-Type' : 'application/x-www-form-urlencoded' },
                params: tasks
            });
        },
        submitTask: function(taskData) {
            console.log(taskData);
            return $http({
                method: 'POST',
                url: '/api/tasks',
                headers: { 'Content-Type' : 'application/x-www-form-urlencoded' },
                params: taskData
            });
        },
        updateTask: function(taskData) {
            return $http({
                method: 'PUT',
                url: '/api/tasks/' + taskData.id,
                headers: { 'Content-Type' : 'application/x-www-form-urlencoded' },
                params: taskData
            });
        },
        deleteTask: function(id) {
            return $http.delete('/api/tasks/' + id);
        }
    };
}).
controller('ProjectController', ['$scope', '$route', '$http', 'Project', 'projectService', function($scope, $route, $http, Project, projectService) {

    $scope.project = null;
    $scope.userID = 1;
    $scope.projectData = {};

    $scope.getUserProjects = function(setProjectId) {
        projectService.getUserProjects($scope.userID).success(function(data) {
            $scope.projects = data;
            $scope.setProject($scope.projects[0].id);
        });
    }

    $scope.setProject = function(id) {
        projectService.setProjectId(id);
    };

    $scope.isCurrentProject = function(id) {
        return id === projectService.projectID;
    }
    $scope.submitProject = function() {
        $scope.projectData.user_id = $scope.userID;
        projectService.submitProject($scope.projectData)
            .success(function(data) {
                $scope.projectData = {};
                $scope.getUserProjects();
            });
    }
    $scope.deleteProject = function(id) {
        projectService.deleteProject(id).success(function(data) {
            $scope.getUserProjects();
        });
    }
    $scope.getUserProjects();
}]).
controller('TaskController', ['$scope', '$http', 'projectService', function($scope, $http, projectService){

    $scope.taskData = {};

    $scope.$watch( function() { return projectService.projectID }, function( projectID ) {
        if(projectID) {
            projectService.getProjectTasks(projectID).success(function(data) {
                $scope.tasks = data;
            }); 
        }
    });
    $scope.submitTask = function() {
        $scope.taskData.project_id = projectService.projectID;
        projectService.submitTask($scope.taskData)
            .success(function() {
                $scope.taskData = {};
                projectService.getProjectTasks().success(function(data) {
                    $scope.tasks = data;
                }); 
            });
    }
    $scope.updateTask = function(taskData) {
        projectService.updateTask(taskData);
    }
    $scope.deleteTask = function(id) {
        projectService.deleteTask(id).success(function() {
            projectService.getProjectTasks().success(function(data) {
                $scope.tasks = data;
            });
        });
    }
    $scope.sortableOptions = {
        update: function(e, ui) {
            projectService.updateProjectTasks($scope.tasks);
        }
    }
}]);

HTML Body

<body>
<div class="col-md-4">
  <div ng-controller="ProjectController as projectCtrl">
    <h2>Projects</h2>
    <%loading%>
    <ul class="list-group list-unstyled">
      <li ng-repeat="project in projects" class="input-group">
        <a class="list-group-item form-control" href ng-class="{'active' : isCurrentProject(project.id)}" ng-click="setProject(project.id)"><%project.name%></a>
        <span class="input-group-btn">
          <button class="btn btn-danger" ng-click="deleteProject(project.id)">
          <span class="glyphicon glyphicon-remove"></span>
          </button>
        </span>
      </li>
    </ul>
    <hr />
    <form ng-submit="submitProject()" role="form">
      <div class="form-group">
        <input type="text" ng-model="projectData.name" placeholder="Project name" class="form-control"/>
      </div>
      <button type="submit" class="btn btn-primary pull-right">
        <span class="glyphicon glyphicon-folder-open"></span>
        &nbsp;&nbsp;Add Project
      </button>
    </form>
  </div>
</div>

<div class="col-md-8">
  <div ng-controller="TaskController as taskCtrl">
    <h2>Tasks</h2>
    <form ng-submit="submitTask()" role="form">
      <div class="input-group">
        <input type="text" ng-model="taskData.name" placeholder="Task name" class="form-control"/>
        <span class="input-group-btn">
          <button type="submit" class="btn btn-primary pull-right">
            <span class="glyphicon glyphicon-plus"></span>
            &nbsp;&nbsp;Add Task
          </button>
        </span>
      </div>
    </form>
    <hr />
    <table class="table table-hover table-striped">
      <thead></thead>
      <tbody ui-sortable="sortableOptions" ng-model="tasks">
        <tr ng-repeat="task in tasks">
          <td>
            <div class="checkbox">
              <label ng-class="{'complete' : task.complete}">
                <input type="checkbox" ng-checked="task.complete" ng-model="task.complete" ng-change="updateTask(task)"/><%task.name%></label>
              </div>
          </td>
          <td><button class="btn btn-warning pull-right" ng-click="deleteTask(task.id)"><span class="glyphicon glyphicon-minus"></span></button></td>
        </tr>
      </tbody>
    </table>
  </div>
</div>
</body>
share|improve this question
up vote 2 down vote accepted

Some advice:

  1. Separate your controllers, factories/services, etc, each in one different file.

  2. Organize your project by feature instead of type.

  3. The controllers should be thin. It's responsible for presentation logic of your application. Something like viewModel pattern.

  4. Put into your controllers only is necessary to send back to your views.

  5. NEVER maipulate DOM in your controllers. Instead use directives and services.

  6. Send your business logic to factories/services.

I highly advise to read this links on the gist:

  1. https://gist.github.com/cbfranca/fd68ce99bd0082131500

I hope this has helped you.

share|improve this answer

Your projectService is not a factory. You are defining a factory in your application that returns a projectService object. This is the correct pattern to follow.

Reference: AngularJS: Developer Guide: Services

I'm not seeing an architectural problem here.

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.