5
\$\begingroup\$

I am still learning the javascript language.

Recently I've written simple code for a task list. I am searching for improvements and advice from more experienced users. Are there any improvements, changes or specific patterns for this task?

PS- I wanted to use pure JavaScript (e.g. no jQuery or AngularJS) as this is only for learning.

The complete source code can be found at jsfiddle.

JS code:

var scheduleListApp = (function() {
    'use strict';

    var helpers = {},
        listArr = ['My birthdays', 'Take a pill', 'Eat something'],
        elems = {
            arrayContainer : document.getElementsByClassName('container')[0],
            inputTask : document.getElementsByClassName('task')[0],
            inputAdd : document.getElementsByClassName('addTask')[0],
            inputDelete : document.getElementsByClassName('deleteTask')[0],
            submitForm : document.getElementsByTagName('form')[0],
            listNumber : document.getElementsByClassName('number')[0].getElementsByTagName('span')[0]
        },
        inserted = false;

    helpers = {
        readFromArr : function() {
            return listArr.length;
        },
        clearList : function() {
            if(elems.arrayContainer.hasChildNodes()) {
                console.log('ready for cleaning.');
                elems.arrayContainer.removeChild(elems.arrayContainer.firstChild);
            }
        },
        displayArr : function() {
            var tempArr = document.createDocumentFragment().appendChild(document.createElement('ul')),
                list = listArr.length,
                i = 0,
                number = this.readFromArr();

            if(inserted === true) {
                console.log('Array is loaded.');
                return false;
            }

            this.clearList();

            for(;i < list; i+=1) {
                var elem = document.createElement('li');

                elem.textContent = listArr[i];
                elem.insertAdjacentHTML('afterBegin', '<input type="checkbox">');

                tempArr.appendChild(elem);
            }

            elems.arrayContainer.appendChild(tempArr);
            inserted = true;
            this.updateNumber(number);

            return elems.arrayContainer;
        },
        updateNumber : function(number) {
            return elems.listNumber.textContent = number;
        },
        submitProxy : function(event) {
            event.preventDefault();

            if(document.activeElement === elems.inputAdd) {
                return this.addTask(event);
            }

            return this.deleteTask(event);
        },
        addTask : function(e) {
            var newValue = elems.inputTask.value,
                oldValue = listArr[listArr.length-1];
                console.log(e, oldValue);

            if(newValue === oldValue || newValue === "") {
                console.log('Please enter new value.');
                return false;
            }

            this.displayNewTask(newValue);
        },
        emptyArr : function() {
            var elem = elems.arrayContainer;

            if(listArr.length === 0) {
                elem.innerHTML = '<p>There is no tasks added</p>';
            }
        },
        deleteTask : function(e) {
            var elem = document.querySelectorAll('input[type="checkbox"]'),
                i = 0,
                j = 0,
                list = elem.length,
                toDelete = [];

            for(; i < list; i+=1) {

                if(elem[i].checked === true) {
                    toDelete.push(i);
                    //toDelete.push(elem[i].nextSibling.nodeValue);
                }
            }

            for(; j < toDelete.length; j+=1) {
                listArr.splice(toDelete[j] - j,1);
            }

            console.log('Array to delete: ', listArr);

            if (toDelete.length === 0) {
                console.log('Nothing to delete.');
                return false;
            }

            inserted = false;
            this.displayArr();
            this.emptyArr();

        },
        displayNewTask : function(task) {
            var number = this.readFromArr();

            listArr.push(task);

            inserted = false;
            this.updateNumber(number);
            this.displayArr();
        }
    }

    elems.submitForm.addEventListener('submit', helpers.submitProxy.bind(helpers), true);
    window.addEventListener('load', helpers.displayArr.bind(helpers), false);

    return {
        readFromArr : helpers.readFromArr,
        displayArr : helpers.displayArr
    }

})();
\$\endgroup\$
  • \$\begingroup\$ I sometimes hear that using DOM elements inside a closure can lead to memory leaks and one much set them to null to have them garbage collected. I;m curious if this is true and if the above code has a potential for a memory leak. Could someone enlighten me? \$\endgroup\$ – Sukima May 4 '14 at 4:25
  • \$\begingroup\$ Please… developer.mozilla.org/en-US/docs/Web/HTML/Element/label \$\endgroup\$ – bjb568 May 5 '14 at 19:15
5
\$\begingroup\$

From a once over

  • Do not select by class name ( as per Jeremy ), I would suggest to setting id's and select by id

    HTML: <input type="text" value="" class="task" id="taskInput"/>
    JS: inputTask : document.getElementsById('taskInput'),

  • 'use strict' is good

  • Your event handling is too cumbersome
    • I would get rid of of these :
      • submitForm : document.getElementsByTagName('form')[0],
      • The function submitProxy
      • elems.submitForm.addEventListener('submit'..
    • I would then simply go for
      • elems.inputAdd.addEventListener('click',helpers.addTask, false); and
        elems.inputDelete.addEventListener('click',helpers.deleteTask, false);
    • Though really, I would probably not group elements under elems and prefix them with $, I would also not group the helpers under helpers:
      • $addButton.addEventListener('click',addTask, false); and
        $deleteButton.addEventListener('click',deleteTask, false);
  • Your naming is odd, sometimes too verbose, sometimes not verbose enough, these are my suggestion:
    • helpers -> don't use that any more
    • listArr -> I would suggest items or tasks
    • elems -> don't use that any more
    • arrayContainer -> $items ?
    • inputTask -> $input ?
    • inputAdd -> addButton ?
    • inputDelete -> deleteButton
    • submitForm -> don't use that any more
    • readFromArr -> This gets the count of items, how about itemsCount or simply removing the whole function since it's a one liner that you are already not using for emptyArr
    • clearList -> This, is where you could have create a todo-list model class. And have clear be a function of that class.
    • displayArr -> Your naming is not consistent, either clearList and displayList or clearArr and displayArr. I prefer xList by far, or you could have gone for xTasks
    • submitProxy - don't use that any more

Other than that, there are few small items:

  • Run JsHint in JsFiddle, you have some missing semi colons
  • Don't use console.log in your final code
  • Don't keep commented out code

I kind of rewrote the app : http://jsfiddle.net/konijn_gmail_com/L7emH/

The code employs a model/view/controller approach to keep things separated/cleaner.

var tasksApp = (function () {
    'use strict';

    var tasks = ['My birthdays', 'Take a pill', 'Eat something'],
        $header = document.getElementById('header'),
        $tasks = document.getElementById('tasks'),
        $input = document.getElementById('input'),
        $addButton = document.getElementById('addButton'),
        $deleteButton = document.getElementById('deleteButton');

    function fillTemplate(s) {
        //Replace ~ with further provided arguments  
        for (var i = 1, a = s.split('~'), s = ''; i < arguments.length; i++)
            s = s + a.shift() + arguments[i];
        return s + a.join("");
    }

    var model = {
        add: function (task) {
            tasks.push(task);
        },
        delete: function (index) {
            if (index > -1) {
                tasks.splice(index, 1);
            }
        },
        getTasks: function () {
            return tasks || [];
        },
        taskPrefix: 'task_'
    };

    var ui = {
        update: function (model) {
            var tasks = model.getTasks(),
                title = 'Task list (' + tasks.length + ')',
                html = '';
            $header.innerText = title;
            for (var i = 0; i < tasks.length; i++) {
                html += fillTemplate('<li><input type="checkbox" id="task_~">~</li>', i, tasks[i]);
            }
            //Using innerHTML might be a bit controversial, but oh so convenient
            $tasks.innerHTML = '<ul>' + html + '</ul>'
        }
    };
    //The controller does the initialization, button event binding and handles events
    var controller = {
        initialize: function () {
            ui.update(model);
            $addButton.addEventListener('click', controller.add);
            $deleteButton.addEventListener('click', controller.delete);
        },
        add: function (e) {
            model.add($input.value);
            ui.update(model);
        },
        delete: function (e) {
            var checkboxes = document.getElementsByTagName('input'),
                checkbox, idParts;
            //We know it is a task checkbox if the id starts with task_
            //Not the perfect design decision but okay for this app
            for (var i = 0; i < checkboxes.length; i++) {
                checkbox = checkboxes[i];
                idParts = checkbox.id.split(model.taskPrefix);
                if ( idParts.length > 1 && checkbox.checked ) {
                    model.delete(+idParts[1]);
                }
            }
            ui.update(model);
        }
    }
    //Bind all al controllers when the document is loaded
    window.addEventListener('load', controller.initialize, false);
    //For giggles, return model, ui and controller
    return {
        model: model,
        ui: ui,
        controller: controller
    }
})();
\$\endgroup\$
  • \$\begingroup\$ konijn Your answer and refactoring my code is such a big thing that I must spend more time on it to analyse. Anyway that's impressive answer! \$\endgroup\$ – cachaito May 5 '14 at 22:55
5
\$\begingroup\$

First reading -- a couple of notes.

Using classnames as selectors

For a long time, using class-names as JavaScript selectors was the common and accepted practice. For a solo-developer, this often works well enough.

However, when developing as a team when someone else is responsible for managing the CSS and layout, it is not uncommon to have CSS class names removed from the markup, thus breaking the site.

There are some common approaches as below:

  • Use a js- prefix on CSS class names as a warning to the designers that some class names are to be left alone.
  • Use the new data-* attributes.
  • Use the form element's name attribute
  • Use IDs.

All of these have pros and cons and without seeing your whole application, I couldn't steer you towards the best of these choices.

The use of the Helpers object

This is, of course, far more subjective.

But, since you already have everything wrapped up in a module thanks to the IIFE, your use of the helpers object is just a bit of over-plumbing. No reason not to have those separate stand-alone functions, free of their own name space.

When reading through, I was surprised to find the use of this that wasn't part of a typical constructor pattern and I had to scroll down to the invocation to find your bind statement.

If you really wanted to keep it is own object, then I'd fall back to the standard prototypical pattern, including capitalizing the constructor name, to make a bit of mental burden off the reader.

\$\endgroup\$
  • \$\begingroup\$ resuming your comment: 1) better class naming 2) when using IIFE, use function statement instead methods 3) unnecesary 'this' (I agree!) \$\endgroup\$ – cachaito May 4 '14 at 18:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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