4
\$\begingroup\$

I am learning to code by making a todo app in pure javascript. My first attempt can be seen here.

It's all cosmetic, meaning it stores no information, generates no objects or arrays, though it was sure fun to learn how to manipulate DOM elements in pure JavaScript and building my first "program"!

I am rebuilding it in a more OOP manner. In this first jsfiddle:

when(toDo, "keypress", function(event){
  if (event.key == "Enter" || event.keyCode == 13) {
    pushArray();
    toDo.value = "";
  }
});

function pushArray(){
  var nt = new Task(toDo.value, "No note yet");
  fruits.push(nt);
  var task = document.createElement("div");
  task.setAttribute("class", "taskitem");
  task.innerHTML = nt.Name;
  var t = document.createElement("input");
  t.setAttribute("class", "tasknote");
  task.appendChild(t);
  when(t, "keypress", function(event){
  if (event.key == "Enter" || event.keyCode == 13) {
    nt.Note = t.value;
    this.parentNode.innerHTML = nt.Name + " " + nt.Note;
    t.value = "";
  }
});

  result.appendChild(task);
  console.log(fruits);
}

Hitting enter fires a function that does everything. creates a new task object, pushes it to the array, displays the task and appends a note that can be updated anytime, for example - task:get milk note:almond milk. However I eventually want to rebuild this app again using local storage, and again with ajax and JSON. I'm thinking I'm going to have a hard time injecting that functionality into this single function without breaking it. So I did it again and broke the steps down into several smaller functions that 'plug' into each other: https://jsfiddle.net/8wm0r345/

  function Task(name, note) {
  this.Note = note;
  this.Name = name;
  this.completed = false;
}

function pushArray() {
  var nt = new Task(toDo.value, "No note yet");
  taskArray.push(nt);
  displayArray(result, nt.Name);
  appendNote(result, nt);
}

function displayArray(x, obj) {
  var task = make("div", "class", "taskitem", obj);
  x.appendChild(task);
}

function appendNote(el, obj) {
  var t = make("input", "class", "tasknote");
  el.appendChild(t);
  when(t, "keypress", submitNote.bind(null, obj, t));
}

function submitNote(newTask, theInput) {
  if (event.key == "Enter" || event.keyCode == 13) {
    newTask.Note = theInput.value;
    var n = make("div", "class", "hasNote", newTask.Note);
    var t = theInput;
    t.parentNode.insertBefore(n, t.nextSibling);
    t.parentNode.removeChild(t);
    when(n, "dblclick", function() {
      n.parentNode.insertBefore(t, n.nextSibling);
      n.parentNode.removeChild(n);
    });
  }
}

My question is: What is the recommended approach to building a small program like this? And are there any alarming issues in either code presented here?

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! As a courtesy, please declare your cross-posts. \$\endgroup\$ – 200_success Jan 12 '16 at 22:33
  • 1
    \$\begingroup\$ I would urge you to use descriptive names for your variables instead of "n", "t", etc. \$\endgroup\$ – 76484 Jan 13 '16 at 1:29
  • \$\begingroup\$ Unfortunately this question is off-topic, you need to include all of your code, and not leave things out like fruits. \$\endgroup\$ – Quill Jan 13 '16 at 5:33
  • 1
    \$\begingroup\$ @Quill While it would be better if the question contained a full runnable example, simply posting an excerpt of the code to be reviewed does not make it off-topic, especially when a runnable example is available as a JSFiddle. \$\endgroup\$ – 200_success Jan 13 '16 at 6:21
1
\$\begingroup\$

Please don't use nt and t as variable names. If you were using them in a small limited context then it wouldn't be much of an issue, but repeatedly using them across multiple lines is going to get confusing. What's wrong with newTask and task?

In the case of var nt = new Task(toDo.value, "No note yet"); it's pretty clear what nt is there, but I'm not even sure if t is a task, or a note on the task, or user input. A clear name makes code more readable, and leaves people guessing at your intentions less.

x in displayArray is even worse. Though it is a limited context, you make it seem as though it accepts arbitrary input (x doesn't specify anything) when it clearly needs something that can have a child appended.

That said I personally like the split up functions a lot more. With individual functions, it's easier to read them and see what the functions do. It's easy for steps to get lost in a big megafunction, but when each step is it's own function you can make everything more explanatory.

However the readability principle applies again. I'm not very familiar with JS, so maybe displayArray makes sense for that, but to me I can't see why a display function is appending something to the HTML. Surely it should be another append function?

| improve this answer | |
\$\endgroup\$

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.