JavaScript Todo App

Question

I wrote a todo app with JavaScript and want to have it reviewed to see if I can make it better.

I think my JavaScript is messy and can be improved a lot. Please share your thoughts and way to improve this (I think my JavaScript can be half of the code to do the exact same thing).

Here is my app.

var mainInput = document.getElementById("mainInput");
var totalTodo = document.getElementById('totalTodos');
// Set this to give unique id on each todo list
var todoid = '0';
var todoDone = '0';

// Add todos - Trigger on Enter
mainInput.addEventListener("keyup", function(e){
    var key = e.which || e.keyCode;
    if (key === 13) {
        if (mainInput.value === ''){
            // If empty value entered
            //console.log('Empty value');
            return false;
        }else{
            // If value passed
            todoid++;
            //console.log(todoid);
            // Create element to hold new value
            var node = document.createElement("LI");
            
            // Add data attribute on li
            node.setAttribute('id', 'todo_' + todoid);
            
            // Greate i element - Check icon
            var nodeTwo = document.createElement("I");
            // Add i into li element
            node.appendChild(nodeTwo);
            // Add Classes into i
            nodeTwo.className = 'ion-ios-circle-outline';
            nodeTwo.setAttribute('id', 'child_todo_' + todoid);

            
            // Save textnode from the value
            var textnode = document.createTextNode(mainInput.value);
            // Add that into li
            node.appendChild(textnode);
            
            
            // Greate i element - Remove icon
            var nodeThree = document.createElement("I");
            // Add i into li element
            node.appendChild(nodeThree);
            // Add Classes into i
            nodeThree.className = 'ion-ios-close-empty';
            nodeThree.setAttribute('id', 'close_child_todo_' + todoid);
            
            // Add new list on the main div
            document.getElementById('todoList').appendChild(node);
            // Clear input
            mainInput.value = '';
            //console.log(mainInput.value);
            
            // Update total todos
            
            totalMsg(todoid, todoDone);
            //console.log('Total ' + todoid + ' todos to do');
        }
    }
});

// Fuction to show total message
function totalMsg(todoid, todoDone){
    if(todoid === 1){
        return totalTodos.innerHTML = '<b>' + todoid + '</b> thing to do / <b>' + todoDone + '</b> completed';
    }else{
        return totalTodos.innerHTML = '<b>' + todoid + '</b> things to do / <b>' + todoDone + '</b> completed';
    }
}

// Function to check Class name on elements
function hasClass(element, cls) {
    return (' ' + element.className + ' ').indexOf(' ' + cls + ' ') > -1;
}

/* Completed todo
** http://jsfiddle.net/founddrama/ggMUn/
** http://stackoverflow.com/questions/646628/how-to-check-if-a-string-startswith-another-string
*/
document.querySelector('body').addEventListener('click', function(event) {
    if (event.target.id.startsWith("child_todo_")) {
        // Get clicked item's ID
        var todoClicked = event.target.id;
        //console.log(todoClicked);
        var todoClickedElement = document.getElementById(todoClicked);
        todoClickedElement.classList.toggle('completed');
        
        var parent_tce = todoClickedElement.parentElement.id;
        //console.log(parent_tce);
        var completedList = document.getElementById(parent_tce);
        completedList.classList.toggle('completedlist');
        
        // if todoClickedElement has completed class add or substract
        //console.log(hasClass(todoClickedElement, 'completed'));
        if(hasClass(completedList, 'completedlist')){
            todoDone++;
            totalMsg(todoid, todoDone);
        }else{
            todoDone--;
            totalMsg(todoid, todoDone);
        }
    }
});

// Remove todo
document.querySelector('body').addEventListener('click', function(event) {
    // Select all elements have ID starts with "child_todo_close_"
    if (event.target.id.startsWith("close_child_todo_")) {
        var closeClicked = event.target.id;
        //console.log(closeClicked);
        var closeClickedElement = document.getElementById(closeClicked);
        var parent_cce = closeClickedElement.parentElement.id;
        
        // Update total todo number
        var willBeRemoved = document.getElementById(parent_cce);
        todoid--;
        if(todoDone != 0 && hasClass(willBeRemoved, 'completedlist')){
            todoDone--;
        }
        if(todoid === 0){
            totalTodos.innerHTML = '';
        }else{
            totalMsg(todoid, todoDone);
        }
        
       // Remove clicked todo list
        document.getElementById(parent_cce).remove();
    }
});

*{
    box-sizing: border-box;
    -moz-box-sizing: border-box;
}
html, body{
    font-family: 'Open Sans', sans-serif;
    font-size: 16px;
    color: #222;
}
.todoApp{
    max-width: 400px;
    margin: 0 auto;
}
.totalTodos{
    font-size: 80%;
}
.todoList{
    margin: 0;
    padding: 0;
}
.todoApp li{
    padding: 0;
    list-style: none;
}
.todoApp i{
    width: 30px;
    height: 30px;
    font-size: 24px;
    text-align: center;
    display: inline-block;
    float: left;
    color: #999;
    padding: 0 4px;
    cursor: pointer;
}
.completed:before{
    content: "\f3fe" !important;
}
.completedlist{
    text-decoration: line-through;
    color: #12A74D;
}
.completedlist i:first-child{
    color: #12A74D;
}
.todoInput li:first-child i{
    display: none;
}
.todoApp i:last-child{
    float: right;
}
.todoList{
    margin-top: 10px;
}
.todoList > li{
    background: #f8f8f8;
    padding: 20px 5px;
    border-bottom: 1px solid #eee;
}
/* Form elements */
.todoApp input{
    width: 100%;
    padding: 10px;
    border: none;
    border-bottom: 1px solid #ccc;
    font-size: 16px;
    font-weight: 300;
    outline: none;
}

<html>
    <!--
    Todo app with Javascript
    Created by O (Ohsik Park) Feb/2016

    ** http://www.OhsikPark.com
    ** Feel free to talk me! [email protected] 
    -------------------------------------------------------------

    License: GNU General Public License
    License URI: http://www.gnu.org/licenses/gpl-2.0.html

    Library used:
    - Google Fonts (https://www.google.com/fonts)
    - ionicons (http://ionicons.com/)
-->
<head>
    <title>Todo app with Javascript</title>
    <link type="text/css" rel="stylesheet" href="css/style.css">
</head>
    <body>
        
        <div class="todoApp" id="todoApp">
            <h1>Todo App with Javascript</h1>
            <li id="todoInput">
                <input type="text" name="mainInput" id="mainInput" placeholder="What needs to be done?" />
            </li>
            <p id="totalTodos" class="totalTodos"></p>
            <ul id="todoList" class="todoList"></ul>
        </div>
        
        <link href='https://fonts.googleapis.com/css?family=Open+Sans:400,700' rel='stylesheet' type='text/css'>
        <link href='http://code.ionicframework.com/ionicons/2.0.1/css/ionicons.min.css' rel='stylesheet' type='text/css'>
        <script src="js/todo.js"></script>
    </body>
</html>

Answer 1

This code is really not so bad, however I see some things that could be changed.

Firstly I point out some code-style improvents.

1. 13 as key code seems to be like a magic number (see Unnamed numerical constants). It would be better to create new var called ENTER_KEY_CODE and use it instead of 13. If you used more key codes, then you would create object keyCodes and save code for each needed key: var keyCodes = {ENTER: 12};.

2. It is not obligatory to use strict equality here: if (key === 13) {. So you can use simple ==.

3. If you want to exit from a function, there is no need to return false;. Just use return;.

4. You do it right when cache elements that will be used later:

   var mainInput = document.getElementById("mainInput");
   var totalTodo = document.getElementById('totalTodos');
   

But why did not you cache todoList element document.getElementById('todoList').appendChild(node);?

5. document.querySelector('body') is document.body. You can use the last one.

6. startsWith method belongs to ECMAScript 6. If you do not use any staff that compile your code, you should use methods that are more browser compatible. if (id.startsWith("close_child_todo_")) can be replaced with if (id.indexOf("close_child_todo_") == 0).

7. You add event listeners on body two times ("Completed todo" and "Remove todo"). It would be nice to add one listener, in which call needed functions:

   document.querySelector('body').addEventListener('click', function(event) {
     todoCompleted(event);
     todoRemoved(event);
   });
   

8. You have got a helper function hasClass. It is nice. Also you can create getById function which will be a chortcut for document.getElementById.

9. You should have some conventions in naming variables, element classes / id's. Secondly, name variables by its context, nature. Let's see some examples.

   var node = document.createElement("LI");
   

   In this case node is a general name. body is node, todoApp is node. Though it is clear from code what this mean, it is better to use a semantic name. For instance, todoItem, todoTask. Also it makes sense to call names of element other than simple util variables. Add some prefix or suffix: $todoItem, todoItemEl.

   This also fits for html classes. You have todoInput and mainInput. If I do not look at html code, I will not understand what is the difference between these elements. todoInput could be changed to todoInput-wrap and mainInput to todoInput. When naming html classes, you can follow some guidlines, for example BEM.

10. Follow DRY rule. This become your code more readable.

11. Do not make a lot of nesting blocks with if / else.

     if (key === 13) {
        if (mainInput.value === ''){
            // If empty value entered
            //console.log('Empty value');
            return false;
        }else{
    

    This code could be formatted to the following:

     if (keyCode != keyCodes.ENTER) return;
     if (this.value === '') return;
    

    Or even shorter:

     if (keyCode != keyCodes.ENTER || this.value === '') return;
    

There are more tips here, but I do not want to write them. Moreover, more experienced js developer would write much more tips. What I advice you is to search the typical solution in the internet (or here, on codereview / stackoverflow). For expamle, if you want to chech if an element has class, search "javascript if element has class" or smth like this. Observe more solutions, understand them and choose the one you understood the more.

In the end I suggest some end example of remaked code.

Before start doing any application it will be nice to think about it: what it should do, what element should be there.

I will do todo app. It shoud have a name. Let's call it todo (this name will be used later). The name could be todoApp but app is not obligatory. It does not give us any additional notes. Also the app shoud have real title. In your case - "Todo App with Javascript". Let's call it title. You can choose name.

The app should take an input name from user. For this a simple input element will be fine. The name will be simple input. As no other input presents here the purpose of this element is clear.

When user submit input, the task should appear in another place. This will be collection of tasks. As you already called it list and it is a quite semantic name, this will be. So, list is a wrap where a task will appear. Even when no tasks exists, list exists always.

Now it is time to think about the main element of the app - task. There are several cases how we could call it. The first is task. The second - todo. However the main name todos represent the context and also contains todo part. So the next variant could be item.

In the end we can talk about small elements. This part

1 thing to do / 0 completed could be called as a summary.

So, we have the names of the main elements. I will not comment all proccess. Just show the code I have got in the end.

Html markup:

    <div class="todos">
            <h1 class="todos__title">Todo App with Javascript</h1>
            <div class="todos__input-wrap">
                <input type="text" class="todos__input" placeholder="What needs to be done?" />
            </div>
            <p class="todos__summary">
                <strong class="todos__count"></strong> thing to do / 
                <strong class="todos__done"></strong> completed
            </p>
            <ul class="todos__list"></ul>
        </div>

        <!-- Each id of a template has '_' at the end to mark it as template.
        You can choose another marker or just use 'todos__item-template'. However it is seems like not a proper way as part 'template' does not change from time to time -->
        <script type="template" id="todos__item_">
            <li class="todos__item" id="todos__item-%%id">
                <i class="todos__item__check ion-ios-circle-outline" data-id="%%id"></i>
                %%name
                <i class="todos__item__remove ion-ios-close-empty" data-id="%%id"></i>
            </li>
        </script>

Js code:

/**
 * Each propery which is 'element' starts with '$'
 */

var keyCodes = {
    ENTER: 13
};

var Todos = (function() {
    // t - shortcut for 'this'
    var t = {
        count: 0,
        done: 0,
        templates: {},

        init: function() {
            this.$el = document.getElementsByClassName('todos')[0];
            this.initTemplates();
            this.defineEls();
            this.initEvents();
            this.updateDone(this.done);
        },

        initTemplates: function() {
            this.templates.item = document.getElementById('todos__item_').innerHTML;
        },

        defineEls: function() {
            ['input', 'list', 'summary', 'count', 'done'].forEach(function(name) {
                t.addEl(name);
            });
        },

        addEl: function(elName) {
            this['$' + elName] = this.$el.getElementsByClassName('todos__' + elName)[0];
        },

        initEvents: function() {
            this.$input.addEventListener('keyup', function(ev) {
                var keyCode = ev.which || ev.keyCode;
                if (keyCode == keyCodes.ENTER && this.value !== '') 
                    t.addItem(this.value);
            });

            this.$el.addEventListener('click', function(ev) {
                if (ev.target.classList.contains('todos__item__check'))
                    t.check(ev.target);

                if (ev.target.classList.contains('todos__item__remove'))
                    t.remove(ev.target);
            });
        },

        remove: function(btn) {
            var item = this.get(btn.getAttribute('data-id'));
            item.parentNode.removeChild(item);
            this.updateDone(--this.done);
            if (!--this.count) this.$summary.style.display = 'none';
            this.updateCount();
        },

        check: function(btn) {
            var item = this.get(btn.getAttribute('data-id'));
            if (item.classList.contains('todos__item--completed')) {
                btn.classList.remove('ion-ios-checkmark-outline');
                btn.classList.add('ion-ios-circle-outline');
                item.classList.remove('todos__item--completed');
                this.updateDone(--this.done);
            } else {
                btn.classList.add('ion-ios-checkmark-outline');
                btn.classList.remove('ion-ios-circle-outline');
                item.classList.add('todos__item--completed');
                this.updateDone(++this.done);
            }
        },

        get: function(id) {
            return document.getElementById('todos__item-' + id);
        },

        addItem: function(name) {
            this.renderEl(name);
            this.updateCount(this.count);
            this.$summary.style.display = 'block';
        },

        updateCount: function(count) {
            if (typeof count == 'undefined') count = this.count;
            this.$count.innerText = count;
        },

        updateDone: function(doneCount) {
            if (doneCount < 0) this.done = doneCount = 0;
            this.$done.innerText = doneCount;
        },

        renderEl: function(name) {
            var id = ++this.count;
            var html = this.templates.item.replace(/%%id/g, id).replace('%%name', name);
            this.$list.insertAdjacentHTML('beforeend', html);
        },

    };
    return t;
})();

Todos.init();

And styles:

* {
    box-sizing: border-box;
    -moz-box-sizing: border-box;
}
html, body{
    font-family: 'Open Sans', sans-serif;
    font-size: 16px;
    color: #222;
}

.todos {
    max-width: 400px;
    margin: 0 auto;
}

.todos__input-wrap {
    padding: 0;
    list-style: none;
}

.todos__input {
    width: 100%;
    padding: 10px;
    border: none;
    border-bottom: 1px solid #ccc;
    font-size: 16px;
    font-weight: 300;
    outline: none;
}

.todos__summary {
    font-size: 80%;
    display: none;
}
.todos__list {
    margin-top: 10px;
    margin: 0;
    padding: 0;
}

.todos__item {
    background: #f8f8f8;
    padding: 20px 5px;
    border-bottom: 1px solid #eee;
    list-style: none;
}
.todos__item--completed {
    text-decoration: line-through;
    color: #12A74D;
}
.todos__item--completed .todos__item__remove {
    color: #12A74D;
}

.todos__item__remove,
.todos__item__check {
    width: 30px;
    height: 30px;
    font-size: 24px;
    text-align: center;
    display: inline-block;
    float: left;
    color: #999;
    padding: 0 4px;
    cursor: pointer;
}
.todos__item__remove {
    float: right;
}

While this code is far from perfection, it has visible improvements.

Firstly, I use templates. Creating nodes directly is fine, but it is difficult to read such a code. When you have templates, you can made much more manipulation easely. Also it allows to use data. I made own function which parse template, but in real app it is preferred to use template engine (handlebars, mustache, jade, nunjucks).

Secondly, I separated blocks to methods. Now it has takes quite a little time to discover what happens in the code.

Now you can write your own example of the new app. Hope this will be helpfull.

Answer 2

Your code doesn't look bad at all. But comments such as

// If empty value entered

// Create element to hold new value

etc.. are bad, and make your code harder to read. It's obvious that
mainInput.value === '' checks if mainInput is empty don't write comment saying it. Also don't name your variables like that:

var nodeTwo = document.createElement("I");
var nodeThree = document.createElement("I");

Better make them more descriptive. However

var textnode = document.createTextNode(mainInput.value);

Is good name. I recommend changing the names like this:

nodeTwo - nodeIonCircle
nodeThree - nodeIonClose