3
\$\begingroup\$

This is my Todo script. It's working well.

I'm looking for feedback on what I've done well, and what I could have done better.

todos = {
  // It needs to store todos
  todos: [],
  // Settings
  settings: {
    quickDelete: false
  },
  // Target container
  targetElement: document.getElementById("todos"),
  // Input txt box
  inputBox: document.getElementById("todo_input"),
  // Priority checkboxes - returns checked radio element
  priorityCheck: function() {
    var inputs = document.getElementsByTagName('INPUT');
    var filtered;
    Array.prototype.forEach.call(inputs, function(input) {
      if (input.type === "radio" && input.checked === true) {
        filtered = input;
      }
    });
    return filtered;
  },
  // Input button
  inputButton: document.getElementById("add_edit_button"),
  // Input label
  label: document.getElementById("add_edit_label"),

  // TOOLS
  // Grammar tools
  grammar: {
    fullStop: function(string) {
      if (string.slice(-1) === ".") {
        return string;
      } else {
        return string + ".";
      }
    },
    capitalise: function(string) {
      var character = string.slice(0, 1);
      if (character === character.toUpperCase()) {
        return string;
      } else {
        return string.slice(0, 1).toUpperCase() + string.slice(1);
      }
    }
  },
  // Create date string from date object
  formatDate: function(creationDate) {
    var lastDigit = creationDate.date.toString().split('').pop();
    var days = ["sun", "mon", "tue", 'wed', "thurs", "fri", "sat"];
    var months = ["jan", "feb", "march", "april", "may", "june", "july", "aug", "sep", "oct", "nov", "dec"];
    var ordinalInd = "";

    // Ordinal Indicator
    if (lastDigit == 1) {
      ordinalInd = "st";
    } else if (lastDigit == 2) {
      ordinalInd = "nd";
    } else if (lastDigit == 3) {
      ordinalInd = "rd"
    } else {
      ordinalInd = "th";
    }

    return `${this.grammar.capitalise(days[creationDate.day])} ${creationDate.date + ordinalInd} ${this.grammar.capitalise(months[creationDate.month])} ${creationDate.year}`
  },
  // It needs to create todos
  createTodo: function() {
    var createDate = new Date();
    var textInput = this.inputBox;
    var grammared = "";
    var priority = this.priorityCheck();
    var lowCheck = document.getElementById('low');
    var creationDate = {
      day: createDate.getDay(),
      date: createDate.getDate(),
      month: createDate.getMonth(),
      year: createDate.getFullYear()
    };
    var pValues = {
      high: 2,
      medium: 1,
      low: 0
    }
    if (textInput.value.length > 0) {
      // Grammarfy
      grammared = this.grammar.fullStop(textInput.value);
      grammared = this.grammar.capitalise(grammared);

      this.todos.push({
        todo: grammared,
        complete: false,
        creationDate: creationDate,
        priority: {
          pName: priority.value,
          pValue: pValues[priority.value]
        }
      });

      textInput.value = "";
      lowCheck.childNodes[0].checked = true;

      this.displayTodos();
    } else {
      alert("Todo cannot be blank!")
    }
  },
  // It needs to edit todos
  editTodo: function(todoIndex) {
    var replaceTxt = this.inputBox.value;
    var lowCheck = document.getElementById('low');
    var pValues = {
      high: 2,
      medium: 1,
      low: 0
    }
    // Grammerfy
    replaceTxt = this.grammar.fullStop(replaceTxt);
    replaceTxt = this.grammar.capitalise(replaceTxt);

    this.todos[todoIndex].todo = replaceTxt;
    this.todos[todoIndex].priority.pName = this.priorityCheck().value;
    this.todos[todoIndex].priority.pValue = pValues[this.priorityCheck().value];

    this.displayTodos();
    // Reset everything
    todos.inputBox.value = "";
    todos.label.innerText = "Add Todo";
    todos.inputButton.innerText = "Add";
    todos.inputButton.setAttribute("onclick", "todos.createTodo()");
    lowCheck.childNodes[0].checked = true;
  },
  // It need to delete todos
  deleteTodo: function(todoIndex) {
    // check if quickdelete setting is true.
    if (this.settings.quickDelete) {
      this.todos.splice(todoIndex, 1);
    } else {
      var deleteIt = confirm("Are you sure you want to delete todo: '" + this.todos[todoIndex].todo + "'?");
      if (deleteIt) {
        this.todos.splice(todoIndex, 1);
      }
    }
    this.displayTodos();
  },
  // Toggle complete todos
  toggleTodo: function(todoIndex) {
    if (this.todos[todoIndex].complete) {
      this.todos[todoIndex].complete = false;
    } else {
      this.todos[todoIndex].complete = true;
    }

    this.displayTodos();
  },
  // Mark all complete
  markAll: function() {
    this.todos.forEach(function(todo) {
      todo.complete = true;
    });

    this.displayTodos();
  },
  // Delete all complete
  deleteComplete: function() {
    var incompleteTodos = [];

    this.todos.forEach(function(todo) {
      if (todo.complete === false) {
        incompleteTodos.push(todo);
      }
    });

    this.todos = incompleteTodos;

    this.displayTodos();
  },
  // Delete all
  deleteAll: function() {
    if (this.settings.quickDelete) {
      this.todos = [];
    } else {
      var deleteAll = confirm("Are you sure you want to delete ALL todos?");
      if (deleteAll) {
        this.todos = [];
      }
    }
    this.displayTodos();
  },
  // Store and retrieve todos locally
  localStore: function(todoArray) {
    // Store
    if (todoArray) {
      localStorage.setItem("todos", JSON.stringify(todoArray));
      // Retrieve
    } else {
      return JSON.parse(localStorage.getItem("todos"));
    }
  },
  // It needs to display todos
  displayTodos: function() {
    console.log("Display todos executed");
    var todoList = this.targetElement;
    todoList.innerHTML = "";

    // table header - if no todos don't show.
    if (this.todos.length > 0) {
      todoList.innerHTML = "<tr class='list_tr'><th></th><th>Todo</th><th>Creation Date</th><th>Priority</th><th>Toggle Done</th><th>Delete</th><th>Edit</th></tr>";
    } else {
      return;
    }

    //console.log("before", this.todos);
    // Sort by priority
    this.todos.sort(function(a, b) {
      return b.priority.pValue - a.priority.pValue;
    });

    //console.log("after", this.todos);
    this.todos.forEach(function(todo, index) {
      console.log("count:", index);
      var listTR = document.createElement('TR');
      var numTD = document.createElement('TD');
      var todoTD = document.createElement('TD');
      var dateTD = document.createElement('TD');
      var priorityTD = document.createElement('TD');
      var completeTD = document.createElement('TD');
      var delTD = document.createElement('TD');
      var editTD = document.createElement('TD');

      var numTxt = document.createTextNode(index + 1 + ".");
      var todoTxt = document.createTextNode(todo.todo);
      //date
      var dateTxt = "";
      if (typeof todo.creationDate === "undefined") {
        dateTxt = "No Creation Date";
      } else {
        dateTxt = todos.formatDate(todo.creationDate);
      }
      var completeTxt;
      if (todo.complete) {
        completeTxt = document.createTextNode("\u2714");
      } else {
        completeTxt = document.createTextNode("X");
      }
      var delTxt = document.createTextNode("Del");
      var editTxt = document.createTextNode("Edit");

      listTR.setAttribute("class", "list_tr");

      listTR.append(numTD);
      listTR.append(todoTD);
      listTR.append(dateTD);
      listTR.append(priorityTD);
      listTR.append(completeTD);
      listTR.append(delTD);
      listTR.append(editTD);

      todoTD.setAttribute("class", "list_col list_tds");
      if (todo.priority.pName === "high") {
        priorityTD.setAttribute("class", "high");
      } else if (todo.priority.pName === "medium") {
        priorityTD.setAttribute("class", "medium");
      } else {
        priorityTD.setAttribute("class", "low");
      }
      dateTD.setAttribute("class", "creation_date");
      completeTD.setAttribute("class", "complete_tds");
      delTD.setAttribute("class", "del_col list_tds");
      editTD.setAttribute("class", "edit_col list_tds");

      numTD.append(numTxt);
      todoTD.append(todoTxt);
      dateTD.append(dateTxt);
      completeTD.append(completeTxt);
      delTD.append(delTxt);
      editTD.append(editTxt);

      todoList.append(listTR);
    });
  },
  // Event handlers
  handleEvents: function(targetElement, eventType) {
    targetElement.addEventListener(eventType, function(event) {
      var targetTxt = event.target.innerText;
      var targetIndex = event.target.parentNode.rowIndex - 1;
      var subButtonTxt = document.getElementById("add_edit_button");

      // Toggle done
      if (targetTxt === "X" || targetTxt === "\u2714") {
        todos.toggleTodo(targetIndex);
        // Delete
      } else if (targetTxt === "Del") {
        todos.deleteTodo(targetIndex);
        // edit
      } else if (targetTxt === "Edit") {

        var makeChecked = document.getElementById(event.target.parentNode.childNodes[3].getAttribute("class"))

        makeChecked.childNodes[0].checked = true;
        todos.inputBox.value = event.target.parentNode.childNodes[1].innerText;
        todos.label.innerText = "Edit Todo";
        todos.inputButton.innerText = "Save Edit";
        todos.inputButton.setAttribute("onclick", "todos.editTodo(" + targetIndex + ")");
      } else if (event.keyCode === 13 && subButtonTxt.innerText === "Add") {
        todos.createTodo();
      } else if (event.keyCode === 13 && subButtonTxt.innerText === "Save Edit") {
        subButtonTxt.click();
      }
    });
    // window.addEventListener("load", function() {
    //     console.log("LOAD event fired!");

    // });
    window.addEventListener("unload", function() {
      todos.localStore(todos.todos);
    });
  }
};

todos.handleEvents(todos.targetElement, "click");
todos.handleEvents(window, "keyup");
todos.todos = todos.localStore() || [];
todos.displayTodos();
body,
html {
  font-family: verdana;
  color: #414141;
  background-color: darkgrey;
}

h1 {
  width: 450px;
  font-family: times;
  font-size: 1.8em;
  margin: 10px auto 20px auto;
  text-transform: uppercase;
  letter-spacing: 5px;
  color: lightgrey;
  border-bottom: 1px solid lightgrey;
}

h2 {
  width: 450px;
  font-family: times;
  font-size: 0.8em;
  text-align: center;
  margin: 10px auto 20px auto;
  text-transform: uppercase;
  letter-spacing: 5px;
  color: lightgrey;
}

button {
  border-radius: 5px;
  background-color: white;
}

.del_col {
  text-align: center;
  background-color: rgba(200, 0, 0, 0.3);
  cursor: pointer;
}

.edit_col {
  background-color: rgba(0, 200, 0, 0.3);
  cursor: pointer;
}

.list_col {
  text-align: left;
  background-color: lightGrey;
  min-width: 150px;
}

.list_tds {
  font-size: 0.8em;
  padding: 5px 10px 5px 10px;
  border-radius: 3px;
}

.complete_tds {
  text-align: center;
  background-color: white;
  cursor: pointer;
  border-radius: 3px;
}

.todo_box {
  width: 300px;
  padding: 20px;
  border-radius: 15px;
  background-color: #E1E1E1;
}

.high {
  background: #e20000;
  border-radius: 5%;
}

.medium {
  background: #ffc907;
  border-radius: 5%;
}

.low {
  background: #54bc00;
  border-radius: 5%;
}

.creation_date {
  text-align: center;
  font-size: 0.6em;
}

#todos th:nth-child(n+2) {
  font-size: 0.8em;
  font-weight: 400;
  color: white;
  background-color: darkgrey;
  border-radius: 3px 3px 0px 0px;
  padding: 5px 10px 5px 10px;
}

#add_edit_label {
  color: #414141;
  font-size: 0.8em;
  color: #A1A1A1;
}

#new_todos label {
  font-size: 1em;
  color: #A1A1A1;
}

#new_todos span {
  font-size: 0.8em;
  color: #A1A1A1;
  margin-right: 15px;
}

#todo_input {
  width: 300px;
}

#container {
  padding: 10px;
  margin: 100px 0px 100px 0px;
  width: 580px;
  margin: auto;
  background-color: #F1F1F1;
  border-radius: 20px;
  border: 1px solid grey;
}

#high {
  padding: 0 1px 0 0;
  border: 1px solid #AAA;
  display: inline;
  background: #e20000;
  border-radius: 5px;
}

#medium {
  padding: 0 1px 0 0;
  ;
  border: 1px solid #AAA;
  display: inline;
  background: #ffc907;
  border-radius: 5px;
}

#low {
  padding: 0 1px 0 0;
  ;
  border: 1px solid #AAA;
  display: inline;
  background: #54bc00;
  border-radius: 5px;
}

#new_todos {
  margin-left: 25px;
}

#quickTools li {
  padding-right: 25px;
  display: inline-block;
}

#quickTools ul {
  padding-left: 25px;
  list-style-type: none;
}
<!doctype HTML>
<html>

<head>
  <title>Will's ToDos</title>
  <link rel="stylesheet" type="text/css" href="./style.css">
  <meta charset="utf-8">
</head>

<body>
  <h1>Will's Simple Todo's</h1>
  <h2>This is my todo application, there are many like it, but this one is mine!</h2>
  <div id="container">
    <div id="new_todos">
      <label id="add_edit_label">Add Todo:</label><br>
      <input id="todo_input" type="text"><br>
      <label id="priority_label">Priority:</label><br>
      <div id="high"><input id="high" type="radio" name="priority" value="high"></div><span> High</span>
      <div id="medium"><input id="medium" type="radio" name="priority" value="medium"></div><span> Medium</span>
      <div id="low"><input id="low" type="radio" name="priority" value="low" checked></div><span> Low</span>
      <button id="add_edit_button" onclick="todos.createTodo()"> Add</button>
    </div>
    <br>
    <div id="todo_box">
      <table>
        <tbody id="todos"></tbody>
      </table>
    </div>
    <div id="quickTools">
      <ul>
        <li><button onclick="todos.markAll()">Mark all as complete</button></li>
        <li><button onclick="todos.deleteComplete()">Delete completed</button></li>
        <li><button onclick="todos.deleteAll()">Delete all</button></li>
      </ul>
    </div>
  </div>

  <script src="./todos.js"></script>
</body>

</html>

Here is the live code: Will's Todos

\$\endgroup\$
  • \$\begingroup\$ Greetings & Welcome! This question might get closed, because hitting 'Run code snippet' returns an error. Please fix the provided code so that it works without error, because we only review working code. \$\endgroup\$ – konijn Oct 18 '19 at 11:29
  • \$\begingroup\$ Hello konijn. Thank you. I have added the HTML and CSS to make it work in code snippit. I don't know why it's throwing up a security error, but the script is working now. The working script with no error can be seen where it is hosted also. willstodos.1mb.site \$\endgroup\$ – Cuckoo Oct 18 '19 at 11:53
  • \$\begingroup\$ it's all good, I dont think it will get closed now. \$\endgroup\$ – konijn Oct 18 '19 at 11:54
2
\$\begingroup\$
  • You are missing a number of semicolons (ordinalInd = "rd")
  • todos is global (missing var/let/const)
  • Having a variable called todos with a property called todos looks odd, perhaps call the top variable app or widget?
  • Too many comments are superfluous like

    // Settings
    settings: {
    

    or

    // Input button
    inputButton: document.getElementById("add_edit_button"),
    
  • (string.endsWith('.')) is more readable than (string.slice(-1) === ".")
  • Consider Spartan naming, s for string, c for char, i for integer
  • Leverage the fact that .complete is a boolean

    toggleTodo: function(todoIndex) {
        this.todos[todoIndex].complete = !this.todos[todoIndex].complete
        this.displayTodos();
    },
    
  • Consider Array.filter for deleteComplete
  • Avoid console.log in production code
  • I prefer to keep templates like <tr class='list_tr'><th></th><th>Todo</th><th>Creation Date</th><th>Priority</th><th>Toggle Done</th><th>Delete</th><th>Edit</th></tr> outside of JavaScript, and hidden in the body
  • Consider reading up on MVC to make the separation between data, view and controller cleaner

  • Stick to one naming style in HTML (new_todos vs quickTools), I would stick to lowerCamelCase

  • Don't wire your listeners in your HTML, wire them from within your JavaScript

All in all, this code looks good.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Thank you for your excellent feedback Konijn. I really appreciate your taking the time to review my script and offer good advice. I will adopt all the points you have made, as they all make a lot of sense - I didn't even know about .endWith(), that's extremely useful! You are a credit to Stack Exchange. \$\endgroup\$ – Cuckoo Oct 18 '19 at 16:57

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.