20
\$\begingroup\$

I've been doing a simple implementation of a to-do list to learn how to use Knockout.js. I would like a general review of what I've done so far (not much). It's my first application in JavaScript and I've never been very good at Html and CSS, so feel free to give a lot general good things to do, or not to do.

My application is not just one to-do list, you can have multiple lists. There is no back-end for the moment, so there is no save option. I'm wondering if my actual implementation would be difficult to add a back-end service to store information.

I'm using bootstrap as a CSS framework, so my CSS file is really small.

function Task(data){
 this.title = ko.observable(data.title);
 this.isDone = ko.observable(data.isDone);
 this.editable = ko.observable(false);
}
function ListTask(data){
 this.title = ko.observable(data.title);
 this.tasks = ko.observableArray([]);
 this.editable = ko.observable(false);
}
function ViewModel() {
 var self = this;
 self.listVisible = ko.observable(true);
 self.itemsListVisible = ko.observable(false);
 self.listTask = ko.observableArray([]);
 self.newTaskText = ko.observable();
 self.newListText = ko.observable();
 self.allSelected = ko.observable(false);
 self.tasks = ko.observableArray([]);
 self.addListTask = function(){
 self.listTask.push(new ListTask({title: self.newListText()}));
 self.newListText("");
 };
 self.toggleEditableList = function(list){
 list.editable(!list.editable());
 };
 self.showList = function(list){
 self.listVisible(false);
 self.itemsListVisible(true);
 self.tasks(list.tasks());
 };
 self.backToMenu = function(){
 self.listVisible(true);
 self.itemsListVisible(false);
 self.tasks([]);
 };
 self.addTask = function(){
 self.tasks.push(new Task({title : this.newTaskText()}));
 self.newTaskText("");
 };
 self.remove = function(task){
 self.tasks.remove(task);
 };
 self.selectAll = function(){
 var all = self.allSelected();
 ko.utils.arrayForEach(self.tasks(),function(entry){
 entry.isDone(!all); 
 });
 return true;
 };
 self.toggleEditable = function(task){
 task.editable(!task.editable());
 };
}
ko.applyBindings(new ViewModel());
.margin-top-10{
 margin-top: 10px;
}
.padding-bottom-5{
 padding-bottom: 5px;
}
<!DOCTYPE html>
<html lang="en">
<head>
 <meta charset="utf-8" />
 <title>Simple data bind </title>
 <script src="http://cdnjs.cloudflare.com/ajax/libs/knockout/3.0.0/knockout-min.js"></script>
 <link rel="stylesheet" href="http://netdna.bootstrapcdn.com/bootstrap/3.0.3/css/bootstrap-theme.min.css">
</head>
<body>
 <div class="container">
 <h1>Todo List</h1>
 <div data-bind="visible: listVisible">
 <form data-bind="submit: addListTask">
 <div class="margin-top-10">
 Add list: <input data-bind="value: newListText" placeholder="Humm a new list?" />
 <button type="submit" class="btn btn-primary btn-sm">Add</button>
 </div>
 </form>
 <ul data-bind="foreach : listTask" class="margin-top-10">
 <li class="padding-bottom-5"><span data-bind="text: title, visible: !editable(), click: $parent.toggleEditableList"></span><input data-bind="value: title, visible: editable, hasFocus: editable" /></span> <button class="btn btn-sm" data-bind="click: $parent.showList">Show</button></li>
 </ul>
 </div>
 <div id="listItems" data-bind="visible: itemsListVisible">
 <form data-bind="submit: addTask">
 <div class="margin-top-10">
 Add task: <input data-bind="value: newTaskText" placeholder="What needs to be done?" />
 <button type="submit" class="btn btn-primary btn-sm">Add</button>
 </div>
 </form>
 <div class="row">
 <div class="col-lg-6">
 <table class="table table-hover table-condensed" >
 <thead>
 <tr>
 <th class="col-lg-2"><input type="checkbox" data-bind="click: selectAll, checked: allSelected"/></th>
 <th class="col-lg-5">Title</th>
 <th class="col-lg-5">Delete</th>
 </tr>
 </thead>
 <tbody data-bind="foreach: tasks">
 <tr data-bind="css: { active : isDone()}">
 <td class="col-lg-2"><input type="checkbox" data-bind="checked: isDone" /></td>
 <td class="col-lg-5"><span data-bind="text: title, visible: !editable(), click: $parent.toggleEditable"></span><input data-bind="value: title, disable: isDone, visible: editable, hasFocus: editable" /></td>
 <td class="col-lg-5"><a href="#" data-bind="click: $parent.remove"><span class="glyphicon glyphicon-trash"></span></a></td>
 </tr>
 </tbody>
 </table>
 <button class="btn btn-primary btn-sm" data-bind="click: backToMenu">Back</button>
 </div> 
 </div>
 </div>
 </div>
 <script type="text/javascript" src="script/todo.js"></script>
 <script src="https://code.jquery.com/jquery.js"></script>
 <script src="http://netdna.bootstrapcdn.com/bootstrap/3.0.3/js/bootstrap.min.js"></script>
</body>

200_success
146k22 gold badges190 silver badges479 bronze badges
asked Mar 2, 2014 at 21:45
\$\endgroup\$
2
  • \$\begingroup\$ You can fairly easy save and load files using HTML5, so that would take care of your backend problem. \$\endgroup\$ Commented Mar 5, 2014 at 13:48
  • \$\begingroup\$ @Max Didn't know that, but I will add a backend in later part of my personal project. But it could be a good intermediate if I don't want to add a backend. \$\endgroup\$ Commented Mar 5, 2014 at 13:50

3 Answers 3

6
+50
\$\begingroup\$

From staring a while at your code;

Grokking

  • Your HTML stretches really wide sometimes, for maintainability I would indent more:

    <td class="col-lg-5"><span data-bind="text: title, visible: !editable(), click: $parent.toggleEditable"></span><input data-bind="value: title, disable: isDone, visible: editable, hasFocus: editable" /></td>

    becomes

    <td class="col-lg-5">
 <span data-bind="text: title, visible: !editable(), click: $parent.toggleEditable">
 </span><input data-bind="value: title, disable: isDone, visible: editable, hasFocus: editable" />
</td>

Naming

  • I would have named ListTask -> TaskList
  • You have addTask, but then you have remove, that should have been removeTask
  • addTask() does not add a given task, it creates a new blank task, I would call it addNewTask() or newTask() or addBlankTask
  • listVisible and itemsListVisible are unfortunate, I would go for listsListVisible and taskListVisible, in general I would either call every task a task or an item, otherwise you can mix up the reader
  • I was surprised to find that selectAll really sets the isDone flag for every task, I think that should be reflected in the name. (I wonder also why you would want this)

Organization

  • addTask should have been in the prototype of ListTask/TaskList
  • backToMenu should be part of the controller, but you have to add to ViewModel because of data-bind="click: backToMenu" in the HTML. I do not like this from a maintenance perspective, I would have added the listener through JS so that you can see in 1 place what this function is bound to.
  • Far worse is all the JavaScript ( all be it short one liners ) inside your HTML. having JavaScript in your HTML deprives the maintainers from tools ( no syntax highlighting, linting, breakpoints(!) )

Minutiae

  • JSHint.com cannot find anything wrong with your code

In the end

Call me old fashioned, but you are breaking MVC; your HTML contains far too much controller related functionality. I would not mind maintaining your JavaScript at all, it is pretty good. But I would definitely not want to maintain the app :\

A fiddle is here.

answered Mar 6, 2014 at 20:59
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thanks for the review and for all the advices. I guess that breaking the MVC part could be a result of my understanding so far of knockout. This is my first JavaScript application so it may be cause by my inexperience in this too. I'll take some time to digest all the information! \$\endgroup\$ Commented Mar 7, 2014 at 2:42
7
\$\begingroup\$

Don't have time to do a full review but you probably want to reorganize your style and script loading. This will allow the browser to download the stylesheets concurrently while downloading+executing your scripts.

Also you probably want to load jQuery before knockout as knockout will delegate to the more robust jQuery method where applicable. In the press release for [email protected] they claim you won't have to load jQuery first. Also note, there's no advantage to loading knockout in head afaik as it won't actually apply the templates (.applyBindings) until the content ready event.

Also you probably want to point to a single version of jQuery to prevent something going awry between jQuery (quite unlikely I guess but worth considering)..

<head>
 <meta charset="utf-8" />
 <title>Simple data bind </title>
 <link rel="stylesheet" href="http://netdna.bootstrapcdn.com/bootstrap/3.0.3/css/bootstrap.min.css">
 <link rel="stylesheet" type="text/css" href="css/todo.css">
 <link rel="stylesheet" href="http://netdna.bootstrapcdn.com/bootstrap/3.0.3/css/bootstrap-theme.min.css">
</head>
<body>
 <!-- content -->
 <script src="http://code.jquery.com/jquery-1.11.0.js"></script>
 <script src="http://cdnjs.cloudflare.com/ajax/libs/knockout/3.0.0/knockout-min.js"></script>
</body>
answered Mar 5, 2014 at 13:35
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the answer! It still a bit foggy when it come to what should be loaded first and where! This helps in getting it for now. \$\endgroup\$ Commented Mar 5, 2014 at 14:02
  • 1
    \$\begingroup\$ Agreed its not always intuitive check out developers.google.com/speed/articles/include-scripts-properly \$\endgroup\$ Commented Mar 5, 2014 at 14:10
-1
\$\begingroup\$

Nice job.

  1. Try to load Todo.html without Todo.js. Such situation could happen after some incorrect changes. In my opinion it is better to show only initially required elements(inputs) by default.

  2. There is no need to use jquery and bootstrap.

  3. It is possible to add duplicate\empty item.

answered Mar 5, 2014 at 10:13
\$\endgroup\$
2
  • 1
    \$\begingroup\$ There is no need for the JavaScript part of bootstrap at least :) The styles currently are required. \$\endgroup\$ Commented Mar 5, 2014 at 10:43
  • 1
    \$\begingroup\$ @yarix Welcome to Code Review! I'll try to see what I can do about #1, but I'm not quite sure what should be done. #2 In fact, I need bootstrap for my style and jQuery is required by bootstrap if I remember correctly. #3 Well I have not a clear rule about empty/duplicate items. This was just an exercise to learn knockoutJS, but it is a good point. \$\endgroup\$ Commented Mar 5, 2014 at 13:54

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.