Native JavaScript to-do list app using Jade

Question

For practice and to explore my vanilla JavaScript (no-library) weaknesses, yesterday I made a Todo app, similar to what you see floating around for AngularJS blog demos.

How can I optimize this?

// VanillaJS Todo App with full CRUD

(function crudApp() {
  "use strict";
  // Look ma... no library!
  var itemData = {};
  var getSelector = function(targSelector) {
    for (var i = 0; i < document.querySelectorAll(targSelector).length; i++) {
      return document.querySelectorAll(targSelector)[i];
    }
  }
  var selectors = {
    'textInput': '[data-app-todo-input]',
    'addItem': '[data-app-add-item]',
    'outputZone': '[data-app-list-output]',
    'deleteAllItems': '[data-app-delete-all]',
    'deleteItem': 'data-app-delete-item', // These are used on dynamic click events, so they don't get backets
    'editItem': 'data-app-edit-item'
  };

  var methods = {

    'hasAttribute': function(target, attribute) {
      return target.getAttribute(attribute);
    },

    'addItem': function(item, properties) {
      var normalizedItem = item.toLowerCase();

      if (!itemData[item] && properties) {
        itemData[item] = properties;
      } else if (!itemData[item] && !properties) {
        itemData[item] = true;
      }
      // No overwrites for duplicate items
      else if (itemData[item] === true) {
        alert('Duplicate entry prevented');
      }
      getSelector(selectors.textInput).value = '';
      methods.render(itemData);
    },

    'deleteAllItems': function() {
      itemData = {};
      methods.render(itemData);
    },

    'deleteItem': function(itemName) {
      if (itemData[itemName]) {
        delete itemData[itemName];
        methods.render(itemData);
        methods.clearVal(selectors.textInput);
      }
    },

    'readData': function(dataset, property) {
      if (property) return dataset[property];
      else return dataset;
    },

    'editItem': function(event, attr) {
      var itemName = event.target.getAttribute(attr);

      if (itemData[itemName]) {
        var newItemName = prompt('New todo item?');
        if (newItemName) {
          methods.addItem(newItemName);
          methods.deleteItem(itemName); // Reversed from natural order for UX (no item disappear just from pressing edit)
          methods.render(itemData);
        }
      }
    },

    'render': function(dataset) {
      methods.clearHtml(selectors.outputZone);
      var property;
      var currentData = methods.readData(dataset);
      for (property in currentData) {
        if (currentData.hasOwnProperty(property)) {
          getSelector(selectors.outputZone).innerHTML += methods.issueTemplate(property, 'li');
        }
      }
    },

    'clearHtml': function(target) {
      getSelector(target).innerHTML = '';
    },

    'clearVal': function(target){
      getSelector(target).value = '';  
    },

    'issueTemplate': function(data, elm) {
      return '<' + elm + ' data-app-item="' + data + '">' + data + '<button ' + selectors.deleteItem + '="' + data + '">Completed</button><button ' + selectors.editItem + '="' + data + '">Edit</button></' + elm + '>';
    }

  };

  // This should handle both the edit and delete functionality and also click event on dynamically added elms (both come from same elm type so I use the attribute value to decide which function to implement)

  getSelector(selectors.outputZone).onclick = function(event) {

    if (methods.hasAttribute(event.target, 'data-app-delete-item')) {
      var deleteItemAction = new methods.deleteItem(event.target.getAttribute('data-app-delete-item'));
    } else if (methods.hasAttribute(event.target, 'data-app-edit-item')) {
      var editItemAction = new methods.editItem(event, 'data-app-edit-item');
    }

  }

  // Bind DOM events directly to functionality

  getSelector(selectors.deleteAllItems).onclick = function() {
    var deleteAllItemsAction = new methods.deleteAllItems();
    methods.render(itemData);
  };

  getSelector(selectors.addItem).onclick = function() {
    var addItemAction = new methods.addItem(getSelector(selectors.textInput).value);
  };

  getSelector('['+selectors.deleteItem+']').onclick = function() {
    var deleteItemAction = new methods.deleteItem(getSelector(selectors.textInput).value);
    methods.render(itemData);
  };

})();

And this is the Jade markup:

 #app
  .user-entry
    input(type="text", placeholder="Enter an item to do", data-app-todo-input)
    input(type="submit", value="Add item", data-app-add-item)
    input(type="button", value="Delete all items", data-app-delete-all)
    input(type="button", value="Delete specific item", data-app-delete-item)
  .app-output
    h1 Pure javascript todo tracker
    ul(data-app-list-output)

Here is the working demo (please forgive no style, I am only focused on the scripting at this moment).

** Based off of IE9+ requirements please **

Answer 1

I'll dissect your JS from top to bottom. But first, the overarching stuff: you tagged this question with OOP, but it's not taking advantage of OOP. It would be very natural to have Task and TodoList classes that you could build upon instead of having some giant methods object.

// VanillaJS Todo App with full CRUD

Just a comment that CRUD usually implies persistent storage.

(function crudApp() {
  "use strict";

Upvote for strict mode :-)

  // Look ma... no library!
  var itemData = {};
  var getSelector = function(targSelector) {
    for (var i = 0; i < document.querySelectorAll(targSelector).length; i++) {
      return document.querySelectorAll(targSelector)[i];
    }
  }

You're missing a semicolon—this is a function expression and not a function statement. More importantly, however, this function doesn't make much sense as written.

It returns a single element or undefined if it can't find one. That is almost exactly the behaviour of document.querySelector(), which will return null instead if it doesn't match an element. I would just straight up replace this with document.querySelector.

I'd also like to point out an efficiency issue, even though I'm asking you to get rid of it altogether. The loop condition gets executed every iteration. This means that you're actually performing two querySelectorAll calls to return a single element. This is bad, and it'll get even worse if you try to do something as you iterate through them. Instead, assign the result of document.querySelectorAll() to a variable, to avoid calling it multiple times.

  var selectors = {
    'textInput': '[data-app-todo-input]',
    'addItem': '[data-app-add-item]',
    'outputZone': '[data-app-list-output]',
    'deleteAllItems': '[data-app-delete-all]',
    'deleteItem': 'data-app-delete-item', // These are used on dynamic click events, so they don't get backets
    'editItem': 'data-app-edit-item'
  };

  var methods = {

    'hasAttribute': function(target, attribute) {
      return target.getAttribute(attribute);
    },

    'addItem': function(item, properties) {
      var normalizedItem = item.toLowerCase();

You edited out normalizedItem; you should've taken this line out too.

      if (!itemData[item] && properties) {
        itemData[item] = properties;
      } else if (!itemData[item] && !properties) {
        itemData[item] = true;
      }
      // No overwrites for duplicate items
      else if (itemData[item] === true) {
        alert('Duplicate entry prevented');
      }

I just want to highlight your if-else if chain. You should stick to one style. I understand that you want to put in a comment, but this should not cause you to uncuddle the else. You can put the comment on the line after the { instead.

Also, this function needs to be documented. I've read it over twice and I can't understand why an item has to be === true to count as an item. It doesn't help that you didn't actually use the addItem(item, properties) form ever.

Anyway, I'd rewrite it a bit still:

if (itemData[item]) {
  alert('...');
} else {
  itemData[item] = properties || true;
  /* or
  if (properties === undefined) {
    itemData[item] = true;
  } else {
    itemData[item] = false;
  }
  */
}

You'll also have to think about whether you need to distinguish between properties being undefined, i.e. defaulting to true, and properties being falsey, like 0 or false or ''.

      getSelector(selectors.textInput).value = '';
      methods.render(itemData);

Ideally, you would separate these to be in the event handler, since they deal with UI logic instead, while the rest deal with model logic. This also causes you an issue later on.

    },

    'deleteAllItems': function() {
      itemData = {};
      methods.render(itemData);
    },

    'deleteItem': function(itemName) {
      if (itemData[itemName]) {
        delete itemData[itemName];
        methods.render(itemData);
        methods.clearVal(selectors.textInput);
      }
    },

    'readData': function(dataset, property) {
      if (property) return dataset[property];
      else return dataset;

Prime time to use the conditional operator. Consider rewriting as return property ? dataset[property] : dataset;. (Although, I would still question why you need to return both.

    },

    'editItem': function(event, attr) {
      var itemName = event.target.getAttribute(attr);

      if (itemData[itemName]) {
        var newItemName = prompt('New todo item?');
        if (newItemName) {
          methods.addItem(newItemName);
          methods.deleteItem(itemName); // Reversed from natural order for UX (no item disappear just from pressing edit)

This is a little bit messy; this is the issue I was referring to earlier. It's good that you added a comment, but even with the comment it seems a little bit bleh. This is mostly an issue because your addItem and deleteItem methods both call render, even though in this instance you'd prefer to just use the logic.

If you can, it'd be better to separate the model logic (the stuff dealing with the adding and deleting on itemData) from the presentation logic (rendering, clearing input boxes, etc.). That way this method becomes a lot cleaner.

There's also a bug here: if you edit to an already existing todo item, the add will fail, but the delete will still run, so you lose your existing item. This will even happen when you edit to the same thing.

          methods.render(itemData);
        }
      }
    },

    'render': function(dataset) {
      methods.clearHtml(selectors.outputZone);
      var property;
      var currentData = methods.readData(dataset);
      for (property in currentData) {
        if (currentData.hasOwnProperty(property)) {
          getSelector(selectors.outputZone).innerHTML += methods.issueTemplate(property, 'li');

This is a bit of an inefficient way to render. Each time you modify innerHTML, the DOM is going to change. Instead, it's better to add up your modifications and then only modify once.

var currentData = methods.readData(dataset);
var html = '';
for (var property in currentData) {
  if (currentData.hasOwnProperty(property)) {
    html += methods.issueTemplate(property, 'li');
  }
}
getSelector(selectors.outputZone).innerHTML = html;

In addition, in your code, you're running a lot of document.querySelectorAlls here. Rather than calling it each time, you can store the result into a variable and only have to call it once. I didn't do it here because by making the other change, you'll only have to call it once anyway.

        }
      }
    },

    'clearHtml': function(target) {
      getSelector(target).innerHTML = '';
    },

    'clearVal': function(target){
      getSelector(target).value = '';  
    },

    'issueTemplate': function(data, elm) {
      return '<' + elm + ' data-app-item="' + data + '">' + data + '<button ' + selectors.deleteItem + '="' + data + '">Completed</button><button ' + selectors.editItem + '="' + data + '">Edit</button></' + elm + '>';

You need to escape data, or your app will break if I make a todo item using " characters.

    }

  };

  // This should handle both the edit and delete functionality and also click event on dynamically added elms (both come from same elm type so I use the attribute value to decide which function to implement)

  getSelector(selectors.outputZone).onclick = function(event) {

    if (methods.hasAttribute(event.target, 'data-app-delete-item')) {
      var deleteItemAction = new methods.deleteItem(event.target.getAttribute('data-app-delete-item'));

That's not how the new keyword works. new creates an object using a constructor function, but your methods.deleteItem function does not behave like a constructor. This works only because new will perform the function call, but you're really just doing methods.deleteItem(event.target.getAttribute('data-app-delete-item').

Also, rather than using getAttribute for data attributes, use dataset. This holds for the rest of your code as well.

    } else if (methods.hasAttribute(event.target, 'data-app-edit-item')) {
      var editItemAction = new methods.editItem(event, 'data-app-edit-item');
    }

  }

  // Bind DOM events directly to functionality

  getSelector(selectors.deleteAllItems).onclick = function() {
    var deleteAllItemsAction = new methods.deleteAllItems();
    methods.render(itemData);
  };

  getSelector(selectors.addItem).onclick = function() {
    var addItemAction = new methods.addItem(getSelector(selectors.textInput).value);
  };

  getSelector('['+selectors.deleteItem+']').onclick = function() {
    var deleteItemAction = new methods.deleteItem(getSelector(selectors.textInput).value);
    methods.render(itemData);
  };

})();