4
\$\begingroup\$

This is how my app looks like:

Screenshot

I can add and remove tasks, mark them as complete, move them up and down on a list, and filter them.

I have few questions about some parts of my code that I'm pasting here.

First thing that concerns me is that in addTask() function I have tons of document.createElement one by one and it looks like a wall of code. Is this ok?

Second thing that makes me wonder is that in the same function (addTask) I am appending many things and it feels like I am repeating myself. Is there a cleaner solution?

My third question is about editTask() function. I feel it needs lots of improvement, but I don't really know how to do it. For example two lines are setting my inputButton.style. Should I move them to CSS file and set style just to get one line of code less?

Of course if there are any mistakes please point them out!

const vApp = {

addTask: () => {
    const taskText = document.getElementById('newTask'),
        contentBox = document.getElementById('tasksholder'),
        taskCreator = document.createElement('div'),
        textBox = document.createElement('div'),
        deleteButton = document.createElement('button'),
        editButton = document.createElement('button'),
        downButton = document.createElement('button'),
        upButton = document.createElement('button'),
        checkBox = document.createElement('input'),
        label = document.createElement('label');

    if (taskText.value.length > 0) {        // protection against empty input
        taskCreator.className = 'task-box';
        textBox.innerHTML = taskText.value;
        textBox.className = 'textBox';
        contentBox.insertBefore(taskCreator, contentBox.childNodes[0]);
        taskText.value = '';                // clear input box

        deleteButton.style.backgroundImage = "url('img/delete.png')";
        deleteButton.addEventListener('click', function() {
            contentBox.removeChild(this.parentNode);
        });

        editButton.style.backgroundImage = "url('img/edit.png')";
        editButton.addEventListener('click', function() {
            vApp.disableEditing(this.parentNode, true);
            vApp.editTask(this.parentNode);
        });

        downButton.style.backgroundImage = "url('img/down.png')";
        downButton.addEventListener('click', function() {
            if (this.parentNode ===  contentBox.lastChild) {
                return false;
            } else {
                vApp.swapElements(this.parentNode, this.parentNode.nextSibling);
            }
        });

        upButton.style.backgroundImage = "url('img/up.png')";
        upButton.addEventListener('click', function() {
            if (this.parentNode ===  contentBox.firstChild) {
                return false;
            } else {
                vApp.swapElements(this.parentNode, this.parentNode.previousSibling);
            }
        });

        checkBox.type = "checkbox";
        checkBox.addEventListener('click', function() {
           vApp.manageCheckbox(this.parentNode);
        });

        label.appendChild(checkBox);
        taskCreator.appendChild(textBox);
        taskCreator.appendChild(label);
        taskCreator.appendChild(downButton);
        taskCreator.appendChild(upButton);
        taskCreator.appendChild(editButton);
        taskCreator.appendChild(deleteButton);
        taskText.placeholder = 'What else needs to be done?';

    } else {
        vApp.warn();
    }
},

disableEditing: (buttonParent, enabling) => {
    buttonParent.childNodes[4].disabled = enabling;
},

editTask: (taskToEdit) => {
    const bubble = document.createElement('span'),
        inputField = document.createElement('input'),
        inputButton = document.createElement('button'),
        textContainer = document.createElement('div');

    inputField.value = taskToEdit.innerText;
    inputField.setAttribute('type', 'text');
    inputField.setAttribute('maxlength', '40');
    taskToEdit.removeChild(taskToEdit.firstChild);
    textContainer.className = 'textBox';
    bubble.className = 'bubble';
    inputButton.innerText = 'Ok';
    inputButton.style.fontSize = '18px';
    inputButton.style.color = "#dddddd";
    inputButton.addEventListener('click', function() {
        textContainer.innerHTML = inputField.value;
        taskToEdit.insertBefore(textContainer, taskToEdit.childNodes[0]);
        vApp.disableEditing(taskToEdit,false);
        bubble.parentNode.removeChild(bubble);
    });
    bubble.appendChild(inputField);
    bubble.appendChild(inputButton);
    taskToEdit.appendChild(bubble);
},


};
\$\endgroup\$
2
\$\begingroup\$
  1. If you want, you can always refactor functions like document.getElementById() so that you type less. Something that I see a lot of people do is:

    const E = id => document.getElementById(id);
    
    ...
    
    const taskText = E('newTask'),
    ...
    
  2. Same thing for document.createElement():

    const Element = tag => document.createElement(tag);
    
  3. Anything involving styles should probably be in an external CSS stylesheet.

  4. You can paste your HTML into the question and make a runnable snippet so that we can try out your application.

\$\endgroup\$
  • 1
    \$\begingroup\$ Is 1. and 2. really a good practice? It seems like making code less verbal. \$\endgroup\$ – Yewhral Jul 16 '17 at 7:52
  • \$\begingroup\$ JavaScript is VERY verbal, almost too verbal when it comes to this. You can choose a different name for the alias to make it more readable to you if you want. \$\endgroup\$ – omgimanerd Jul 17 '17 at 18:00

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.