1
\$\begingroup\$

I have written a todo list app. I want to understand how to improve it.

Here's what I'm trying to understand:

My problem is when user clicks on task to edit, because it passed by reference, so if user edit task, it will change directly the task object. (see the code below).

My questions:

  1. In my code I wrote one way to fix the above mentioned problem by cloning the object every time. Is it a good practice ? If no, how do you recommend me to fix it?

  2. I do not want my code to just work, I want it be efficient and improve my skills. Can you please suggest any areas of improvement to the code?

Link to plunker: https://plnkr.co/edit/CA99iiydbD4TWaGtJZZf?p=preview

(function() {
    var Task = (function() {
        var counter = 0;
        return function(content, isDone) {
            this.id = counter++;
            this.content = content;
            this.isDone = isDone || false;
        }
    }());
    var app = angular.module('todos',[])
    .service('todosBL', function() {
        this.tasks = [];
        this.add = function(content) {        
            this.tasks.push(new Task(content));
        }

        this.update = function(editedTask) {
            var i = this.tasks.findIndex(function(task){
                return task.id === editedTask.id
            });
            this.tasks[i] = editedTask;
        }

    })
    .controller('main', function($scope, todosBL) {
        $scope.todosBL = todosBL;
        $scope.add = function(task) {
            todosBL.add(task);
            $scope.task = null;
        }
        $scope.editMode = {
            task: {},
            edit: function(task) {
                this.task = task;
                //BECAUSE I PASS TASK BY REFERNCE WHEN USER EDIT TASK IT CHANGE TASK DIRECTLY ,
                // IF I CLONE OBJECT EVERY TIME, IT FIX BY REFERENCE PROBLEM.
                // MY QUESTION IF HAVE OTHER WAY TO SLOVE THIS. MABY OTHER WAY TO THINK ABOUT APP LIKE THIS.
                // for(key in task) {
                //     this.task[key] = task[key];
                // }
            },
            save: function(task) {
                todosBL.update(task);
                this.task = {};
            }
        };
    });
}());
<html ng-app="todos">
<head>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.min.js"></script>
    <script src="app.js"></script>
</head>
<body>
    <div ng-controller="main">
        <ul>
            <li ng-repeat="task in todosBL.tasks" ng-click="editMode.edit(task)">{{ task.content}}</li>
        </ul>
        <input type="text" ng-model="task">
        <input type="button" value="add task" ng-click="add(task)">
        <!--//for edit-->
        <div>
            <input type="text" ng-model="editMode.task.content">
            <input type="button" value="save task" ng-click="editMode.save(editMode.task)">
        </div>
    </div>
</body>
</html>

\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

Here are a few improvements I can think of.

HTML

As the name of the angular app - todos - is not particularly good. Most developers would follow a convention such as appending 'App' behind the name of the app so that other developers would know that they are looking at a module.

Ref: https://github.com/mgechev/angularjs-style-guide#naming-conventions

<html ng-app="todos">
<head>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.min.js"></script>
    <script src="app.js"></script>
</head>
<body>

As with the name of the app, you could also rename the controller to mainController.

    <div ng-controller="main">
        <ul>

A problem with the list is that use cannot intutively tell that they can click on a task to edit it. You can fix that easily by wrapping an <a> tag around it.

            <li ng-repeat="task in todosBL.tasks"><a href="#" ng-click="editMode.edit(task)">{{ task.content}}</a></li>
        </ul>

Just fixing the case of characters in the button. 

        <input type="text" ng-model="task">
        <input type="button" value="Add Task" ng-click="add(task)">
        <!--for edit-->

Here instead of saying "Save Task" it would be better to change it to "Update Task" as it would more clearly convey the purpose of the button.

        <div>
            <input type="text" ng-model="editMode.task.content">
            <input type="button" value="Update Task" ng-click="editMode.save(editMode.task)">
        </div>
    </div>
</body>
</html>

Javascript:

(function() {
    var Task = (function() {

You could rename the counter variable to better convey its meaning. Maybe even rename the content variable to taskTitle. One more thing, return function(...){...} is a Javascript statement, and although Javascript would not enforce it, you should always append a semicolon ; at the end.

        var taskIdCounter = 0;
        return function(content, isDone) {
            this.id = taskIdCounter++;
            this.content = content;
            this.isDone = isDone || false;
        }; // <-- Semicolon!
    }());
    var app = angular.module('todos',[])
    .service('todosBL', function() {
        this.tasks = [];
        this.add = function(content) {        
            this.tasks.push(new Task(content));
        };

In the controller below, I will make use of the editedTaskId to update the task object from clone. 

        this.update = function(editedTask, editedTaskId) {
          var i = this.tasks.findIndex(function(task){
              return task.id === editedTaskId
          });
          this.tasks[i] = editedTask;
        }
    })

Your logic about cloning the task object to update it is fine. Here's how I'd do it if I had to implement it.

    .controller('main', function($scope, todosBL) {
        $scope.todosBL = todosBL;
        $scope.add = function(task) {
            todosBL.add(task);
            $scope.task = null;
        };
        $scope.editMode = {
            editingTaskId: '',
            task: {},
            edit: function(task) {
                this.task = new Task(task.content, task.isDone);
                this.editingTaskId = task.id;
            },
            save: function(task) {
                todosBL.update(task, this.editingTaskId);
                this.task = {};
            }
        };
    });
}());
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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