1
\$\begingroup\$

I've created my first complete JS app, which lets you add tasks and control how much time you spend on them. I posted this to get some ideas how to improve readability, efficiency and the speed of my code.

The first table shows tasks that you add via input box. Every time you press the "Add task" button, the generateRow() function will be called. If you click start, addSecond will be called every second (setInterval() in my stopTimer function), which will update clock object's "second" property.

The timer will start running and the task will move to "In Progress" table. If you click the "Stop" button, the timer will stop, and the "Stop" button becomes a "Start" button. When you hit "End task", the task will move to "Done" table and the timer will automatically stop. You can delete any task anytime, or click the "Clear table" button above every table to erase all table's data.

The code has some comments to help you understand it better.

let taskName = document.querySelector("#taskName");
let addTask = document.querySelector("#addTask");
let toDoTable = document.querySelector('#toDo');
let inProgressTable = document.querySelector('#inProgress');
let doneTable = document.querySelector('#done');
let clearToDoTable = document.querySelector('#clearToDoTable');
let clearInProgressTable = document.querySelector('#clearInProgressTable');
let clearDoneTable = document.querySelector('#clearDoneTable');

//Eareasing entire table's content by clicking these 'Clear table' buttons
let toDoRows = [];
let inProgressRows = [];
let doneRows = [];

clearToDoTable.addEventListener("click", function(){
	for(let i = 0; i < toDoRows.length; i++){
		toDoRows[i].remove();
	}
	toDoRows.length = 0;
});

clearInProgressTable.addEventListener("click", function(){
	for(let i = 0; i < inProgressRows.length; i++){
		inProgressRows[i].remove();
	}
	inProgressRows.length = 0;
});

clearDoneTable.addEventListener("click", function(){
	for(let i = 0; i < doneRows.length; i++){
		doneRows[i].remove();
	}
	doneRows.length = 0;
});

let generateRow = function(){
	//Creating buttons
	let delToDoRow_btn = document.createElement("button");
	delToDoRow_btn.type = "button";
	delToDoRow_btn.innerText= "Delete";
	delToDoRow_btn.classList.add(".btn");
	delToDoRow_btn.addEventListener("click", deleteToDoRow);

	let delInProgressRow_btn = document.createElement("button");
	delInProgressRow_btn.type = "button";
	delInProgressRow_btn.innerText= "Delete";
	delInProgressRow_btn.classList.add(".btn");
	delInProgressRow_btn.addEventListener("click", deleteInProgressRow);

	let delDoneRow_btn = document.createElement("button");
	delDoneRow_btn.type = "button";
	delDoneRow_btn.innerText= "Delete";
	delDoneRow_btn.classList.add(".btn");
	delDoneRow_btn.addEventListener("click", deleteDoneRow);

	let stop_btn = document.createElement("button");
	stop_btn.type = "button";
	stop_btn.innerText= "Start";
	stop_btn.classList.add(".btn");
	stop_btn.addEventListener("click", stopTimer);

	let end_btn = document.createElement("button");
	end_btn.type = "button";
	end_btn.innerText= "End task";
	end_btn.classList.add(".btn");
	end_btn.addEventListener("click", endTask);

	let tableRowToDo = document.createElement("tr");
	toDoTable.appendChild(tableRowToDo);
	toDoRows.push(tableRowToDo);
	let toDoTableDatas = [];

	//Defining "To do" table
	for(let i = 0;i < 3;i++){
		toDoTableDatas.push(document.createElement("td"));
		tableRowToDo.appendChild(toDoTableDatas[i]);
	}

	toDoTableDatas[0].innerText = taskName.value;
	toDoTableDatas[1].appendChild(stop_btn);
	toDoTableDatas[2].appendChild(delToDoRow_btn);

	//Defining "In progress" table
	let inProgressTableDatas;
	let tableRowInProgress;
	function addInProgressRow() {
		deleteToDoRow();
		tableRowInProgress = document.createElement("tr");
		inProgressRows.push(tableRowInProgress);
		inProgressTable.appendChild(tableRowInProgress);
		inProgressTableDatas = [];

		for(let i = 0;i < 5;i++){
			inProgressTableDatas.push(document.createElement("td"));
			tableRowInProgress.appendChild(inProgressTableDatas[i]);
		}
		inProgressTableDatas[0].innerText = toDoTableDatas[0].innerText;
		inProgressTableDatas[1].appendChild(stop_btn);
		inProgressTableDatas[2].appendChild(end_btn);
		inProgressTableDatas[4].appendChild(delInProgressRow_btn);
	}

	//Defining "Done" table.This function moves data from "In progress" table to "Done" table
	let doneTableDatas;
	let tableRowDone;
	function endTask() {
		deleteInProgressRow();
		tableRowDone = document.createElement("tr");
		doneRows.push(tableRowDone);
		doneTable.appendChild(tableRowDone);
		doneTableDatas = [];

		for(let i = 0;i < 3;i++){
			doneTableDatas.push(document.createElement("td"));
			tableRowDone.appendChild(doneTableDatas[i]);
		}
			doneTableDatas[0].innerText = inProgressTableDatas[0].innerText;
			doneTableDatas[1].innerText = inProgressTableDatas[3].innerText;
			doneTableDatas[2].appendChild(delDoneRow_btn);
	}
	//Creating clock object and related functions
	let clock = {
	stopped: true,
	inProgress: false,

	str_seconds: 0,
	str_minutes: 0,
	str_hours: 0,

	seconds: 0,
	minutes: 0,
	hours: 0,

	clockInterval: null
};

	function addZeroBefore(){
			if(clock.seconds < 10){
				clock.str_seconds = "0" + clock.seconds;
			} else{
				clock.str_seconds = clock.seconds;
			}
			if(clock.minutes < 10){
				clock.str_minutes = "0" + clock.minutes;
			} else{
				clock.str_minutes= clock.minutes;
			}
			if(clock.hours < 10){
				clock.str_hours = "0" + clock.hours;
			} else{
				clock.str_hours = clock.hours;
			}
	}

	function addSecond(){
		 addZeroBefore();
		 if(clock.seconds >= 59){
			clock.seconds = -1;
			clock.minutes++;
		}
		if(clock.minutes > 59){
			clock.minutes = 0;
			clock.hours++;
		}
		inProgressTableDatas[3].innerHTML = "";
		clock.seconds++;

	  inProgressTableDatas[3].innerHTML += clock.str_hours + ":" +clock.str_minutes + ":" + clock.str_seconds;
	}
	//Functions responsible for deletings rows after the button was clicked
	function deleteToDoRow(){
		if(tableRowToDo.parentNode != toDoTable){
			return;
		}
		clock.inProgress  = true;
		toDoTable.removeChild(tableRowToDo);
	}

	function deleteInProgressRow(){
		if(tableRowInProgress.parentNode != inProgressTable){
			return;
		}
		inProgressTable.removeChild(tableRowInProgress);
	}

	function deleteDoneRow(){
		if(tableRowDone.parentNode != doneTable){
			return;
		}
		doneTable.removeChild(tableRowDone);
	}
	//Starting/stopping clock after the button was clicked.This functions also moves a row from "Todo" table to "In progress" table
	function stopTimer(){
		if(clock.inProgress == false){
			addInProgressRow();
		}
		clock.stopped = !clock.stopped;
		if(clock.stopped == false){
			clock.clockInterval = window.setInterval(addSecond, 1000);
			stop_btn.innerText= "Stop";
		} else{
			clearInterval(clock.clockInterval);
			stop_btn.innerText= "Start";
		}
	}
}

addTask.addEventListener("click", generateRow);
<!DOCTYPE html>
<html lang="en-US">
<head>
  <meta charset="utf-8">
  <title>Tasker</title>
  <link href='main.css' rel='stylesheet' type='text/css'>
  <meta name="viewport" content="width=device-width">
</head>
<body>
	<header>
		<h1>Tasker</h1>
	</header>
  <h2>To do</h2>
  <button name="sumbit" type="button" id="clearToDoTable">Clear table</button>
  <form>
    <input type="text" name="name" id="taskName" required>
    <button name="sumbit" type="button" id="addTask">Add task</button>
  </form>
  <table class="tg" id="toDo">
  <tr>
    <th class="tg-yw4l">Name</th>
    <th class="tg-yw4l">Start</th>
    <th class="tg-yw4l">Delete</th>
  </tr>
  </table>
  <h2>In progress</h2>
  <button name="sumbit" type="button" id="clearInProgressTable">Clear table</button>
	<table class="tg" id="inProgress">
  <tr>
    <th class="tg-yw4l">Name</th>
    <th class="tg-yw4l">Start/stop</th>
    <th class="tg-yw4l">End</th>
    <th class="tg-yw4l">Time</th>
    <th class="tg-yw4l">Delete</th>
  </tr>
</table>
<h2>Done tasks</h2>
<button name="sumbit" type="button" id="clearDoneTable">Clear table</button>
<table class="tg" id="done">
<tr>
  <th class="tg-yw4l">Name</th>
  <th class="tg-yw4l">Time</th>
  <th class="tg-yw4l">Delete</th>
</tr>
</table>
<script src="script.js"></script>
</body>
</html>

\$\endgroup\$
  • \$\begingroup\$ A lot of code here. For sure, a todo app not only an add and delete button. I think there are not much people, which want take their time to improve it. Anyway, if you think, there are some parts, which makes you feel bad about it, you should highlight them and ask explicitly what people would suggest to change/improve ;) \$\endgroup\$ – webdeb Jun 19 '16 at 23:33
1
\$\begingroup\$

Please note: your embedded snippet lacks the main.css part declared in <head>.

Usability

Trying to work with your snippet I could use all the features you announced, and they all work as expected.
Nevertheless I noticed several possible improvements, as explained below.

Structure (cosmetic)

The task input area (and its "Add Task" button) would better be placed directly under the application title: so the "To do" part layout will be consistent with the two other ones.

Input area (cosmetic)

It'd be more friendly to clear its previous content after adding a task, so avoiding the need to do it manually before entering a new task.

Task name (important)

Before accepting to add a new task, you should check it's not a duplicate!
(should be checked against both "To do", "In progress", and "Done")

Clear and delete (important)

When the user clicks a "Clear table" or "Delete" button, you should prompt him for confirmation before processing.

Start/Stop (critical)

Currently the user can start any inactive task, both from "To do" or "In progress" parts, even when another one is active: obviously this has no sense!
One solution would be to disable the "Start" button of all inactive task as long as one is active. But to keep the application more friendly you might merely prompt for confirmation, something like "The current task will be stopped. Are you sure?".

Technical aspect

As commented by @webdeb there is a lot of code, so it'd be a real... task :) to review it in detail!
Here are some points.

About best practices

Your code is clean and mostly readable. You only lacked expected spaces in some places, like:

  • almost all cases: for(...){ instead of for (...) {; same for if(...){ and function xxx(){
  • all cases: (in for statements) 0;i < n;i++ instead of 0; i < n; i++
  • sometimes: xxx= yyy instead of xxx = yyy

Reducing code

One simple point is: you may suppress your whole first paragraph of lets.
Like you did in the very last line addTask.addEventListener(...);, all of the document.querySelector(...) may be omitted, since they address an id of the same name.
(you only have to normalize the id of your three tables from <table> to <table>Table to be globally consistent)

At a first glance there are numerous other places where a different strategy might be used to reduce the code (and sometimes to improve performance at the same time).
I only expose two cases below.

The "Clear table" buttons case

let toDoRows = [];
let inProgressRows = [];
let doneRows = [];

clearToDoTable.addEventListener("click", function(){
    for(let i = 0; i < toDoRows.length; i++){
        toDoRows[i].remove();
    }
    toDoRows.length = 0;
});

clearInProgressTable.addEventListener("click", function(){
    for(let i = 0; i < inProgressRows.length; i++){
        inProgressRows[i].remove();
    }
    inProgressRows.length = 0;
});

clearDoneTable.addEventListener("click", function(){
    for(let i = 0; i < doneRows.length; i++){
        doneRows[i].remove();
    }
    doneRows.length = 0;
});

Here we can both:

  • factorize the process for all three tables
  • use a simpler and faster way to clean DOM and the ...Rows arrays

To achieve that, we must first add a common class (say class="clearTable") to the three buttons. This way we can bind their events at once, say to a clearTable() handler.

Then for the clearTable() function to work using the right table and ...Rows array we must also:

  • change the buttons id from clear<table>Table to <table> (remember we already changed the tables ids to <table>Table)
  • group the ...Rows arrays together in a rows object

So finally we can simply write:

let rows = {
  toDo: [],
  inProgress: [],
  done: []
}
for (let item of document.querySelectorAll('.clearTable')) {
  item.addEventListener('click', clearTable);
}
function clearTable(e) {
  let tableName = e.target.id;
  document.querySelector('#' + tableName + 'Table').innerHTML = '';
  rows[tableName] = [];
}

and it replaces the whole previous code part.

The generateButton case

let generateRow = function(){
    //Creating buttons
    let delToDoRow_btn = document.createElement("button");
    delToDoRow_btn.type = "button";
    delToDoRow_btn.innerText= "Delete";
    delToDoRow_btn.classList.add(".btn");
    delToDoRow_btn.addEventListener("click", deleteToDoRow);

    let delInProgressRow_btn = document.createElement("button");
    delInProgressRow_btn.type = "button";
    delInProgressRow_btn.innerText= "Delete";
    delInProgressRow_btn.classList.add(".btn");
    delInProgressRow_btn.addEventListener("click", deleteInProgressRow);

    let delDoneRow_btn = document.createElement("button");
    delDoneRow_btn.type = "button";
    delDoneRow_btn.innerText= "Delete";
    delDoneRow_btn.classList.add(".btn");
    delDoneRow_btn.addEventListener("click", deleteDoneRow);

    let stop_btn = document.createElement("button");
    stop_btn.type = "button";
    stop_btn.innerText= "Start";
    stop_btn.classList.add(".btn");
    stop_btn.addEventListener("click", stopTimer);

    let end_btn = document.createElement("button");
    end_btn.type = "button";
    end_btn.innerText= "End task";
    end_btn.classList.add(".btn");
    end_btn.addEventListener("click", endTask);

Here too we can easily factorize this sequence, like this (hope it's self explanatory):

function generateButton(text, func) {
  let button = document.createElement('button');
  button.type = 'button';
  button.innerText = text;
  button.classList.add('btn');
  button.addEventListener('click', func);
  return button;
}

let generateRow = function(){
    //Creating buttons
  delToDoRow_btn = generateButton('Delete', deleteToDoRow);
  delInProgressRow_btn = generateButton('Delete', deleteInProgressRow);
  delDoneRow_btn = generateButton('Delete', deleteDoneRow);
  stop_btn = generateButton('Start', stopTimer);
  end_btn = generateButton('End task', endTask);

Beyond that, I didn't really explored them but I guess you probably could use better ways when:

  • moving tasks between tables
  • handling start/stop and computing time elapsed
\$\endgroup\$
  • \$\begingroup\$ Most of all, thank you for a thorough answer. I really like the idea to make this app more user-friendly.At first i tackled this by using Javascript's alert() function.Eventually I noticed that alert() stops executing the code,which causes stoppadge of the timer.Would you suggest some way to solve this,or maybe use other method? \$\endgroup\$ – ch3m0 Jun 22 '16 at 20:30
  • \$\begingroup\$ @ch3m0 Oh, you're right: I didn't pay attention to this issue! Then my first thought was to suggest handling your own "dialog box". It implies a lot of additional code for a little work: an overlay <div>covering the whole window with semi-opacity, another overlay <div> as box container with its message and buttons, and the handlers for them... So I suddently realized: after all, when the user just clicked a new task "Start" and while he's responding to the dialog box, _he's not working on the previous task! So using a simple alert() seems not so bad! It's up to you... \$\endgroup\$ – cFreed Jun 22 '16 at 21:36
0
\$\begingroup\$

Readability

In general, readability is improved if your function names are all verbs or verb phrases. Contrariwise, readability is hampered if non-function variables are given verb names. Following these rules of thumb can help "make wrong code look wrong".

In your case, you've named the buttons to clear the tables clearToDoTable, clearInProgressTable, and clearDoneTable. When I see a variable named clearDoneTable my assumption is that I can invoke that method to clear the "done" table, but that's not the case at all... clearDoneTable is not a method but an HTML button.

I suggest making those variables more descriptive, much like you did in your generateRow() function by appending _btn to each of the variable names representing buttons.

Efficiency

When selecting HTML elements by their ID property, browsers are always more efficient using the dedicated document.getElementById("x") method than using the more generic/flexible document.querySelector("#x") method.

Other Thoughts

First, in my experience it's usually best to avoid setInterval altogether in favor of setTimeout. The function you pass to setTimeout can decide whether or not it needs to keep executing and, if so call setTimeout on itself again, and otherwise do nothing. This saves you from having to keep track of an interval ID externally and later call clearInterval on it to stop the process from repeating.

Second (!), rather than attempting to add a second to a counter with every passing second, you can keep a more accurate record of passing time by tracking the exact timestamps when the start and stop buttons are pressed, then calculating the difference as needed. That way, if there is ever a delay in the execution of your JavaScript, your recorded times are still accurate (or at least as accurate as the system clock on the computer executing the JavaScript).

The pseudocode for that logic would be something like this:

set clock.timeElapsed = 0

When start_btn pressed
    set clock.start = current time

Every second while task is active
    update screen to display clock.timeElapsed + (current time - clock.start)

when stop_btn pressed
    set clock.timeElapsed = clock.timeElapsed + (current time - clock.start)
\$\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.