0
\$\begingroup\$

I've created a simple demo that creates new elements on a page. After clicking the button, new elements are created, and when clicking remove, the relevant element is deleted. Each element has a 'data-id' data attribute, which matches the id object property that's also created.

Check out the code on this page, or see this codepen demo I created: https://codepen.io/anon/pen/xMqodd

I'm essentially looking for ways which this code can be improved. Could anyone advise me here? Thanks for any help - the code is below:

HTML:

<button id='btn' onclick='createEntry()'>Create</button>
    <section id='container'></section>

CSS:

button { display: block }
    .entry {
        width: 100%;
        display: block;
        padding: 10px;
        border: 1px solid #f5f5f5;
    }
    span {
        display: block;
        width: 100%;
    }
    section { background: lightgreen }

JS:

// note: beware of using eccessive nested functions as they can cause problems
    var id = 0, objects = [], removes;

    function createEntry() {
        id++;

        // create new element, append to #container & create new object
        var container = document.querySelector('#container'),
            newEntry = document.createElement("div"),
            title = 'title text here',
            description = 'description text here',
            remove = 'remove',
            dataId = id,
            obj = new Entry(title, description, remove);
        newEntry.classList.add("entry");
        newEntry.innerHTML = '<span class="title">' + title + '</span><span class="description">' + description + '</span><span class="remove">' + remove + '</span>';
        container.appendChild(newEntry);
        newEntry.setAttribute('data-id', id);

        updateElements();

        // constructor & push into array
        function Entry(title, description, remove) {
            this.title = title;
            this.description = description;
            this.remove = remove;
            this.id = id;

            objects.push(this);
        }

        // tests
        console.log('JSON.stringify(obj): ' + JSON.stringify(obj));
        console.log('obj.id: ' + obj.id);
    }

    function updateElements() {
        removes = document.querySelectorAll(".remove");
        listenForRemoves();

        function listenForRemoves() {
            for (let remove of removes) {
                remove.removeEventListener("click", removeElements);
                remove.addEventListener("click", removeElements);
            }
        }
    }

    function removeElements() {
        let removedId = this.parentNode.getAttribute('data-id'),
            objToRemove = objects.find(obj => obj.id == removedId); // not used

        this.parentNode.remove(); console.log('removed id ' + removedId);
            console.log('objects before: '); for (let object of objects) { console.log(JSON.stringify(object))};
            objects = objects.filter(obj => obj.id != removedId); // doesn't use objToRemove
            console.log('objects now: '); for (let object of objects) { console.log(JSON.stringify(object))};
    }
\$\endgroup\$
1
\$\begingroup\$

Not sure why you need an id, I think you can do the same without any id.

Even the Entity object array is not clear why you need it.

But the first point is to wrap up your code in a function, so you can contain it safely.

So add a function that contains all the code and just design what you need to pass to that function and what you need to have as a public API from your module.

Another point is about your html:

  <button id='btn' onClick='createEnty()'>Create</button>
  <section id='container'></section>

You should do the bind on the javascript code instead. As you already provided an id for the button, you can easely access to the object and bind on the click event.

So your module can be like so:

function MakeEntryModule(buttonSelector, containerSelector) {
    // code here

    // here the return object with the public methods
    return {
       myMethod: myFunction
    }
}

// then you can do so:
var entryModule = MakeEntryModule('#btn', '#container');

You can also use an anonymous function that is called directly:

var entryModule = (function(a,b){
    // code goes here
})('#btn', '#container');

This is fine, but the previous is more clear when you have parameters.

So here is a rewrite of your code:

function MakeEntryModule(buttonSelector, containerSelector) {
    // cache the container element to avoid 
    // performing the search each time.
    var container = document.querySelector(containerSelector);

    // just pass title and description to the createEntry
    // function to be able to make it more generic.
    function createEntry(titleText, descriptionText) {
        var entry = document.createElement("div");
        entry.classList.add("entry");
        entry.appendChild(createSpan("title", titleText));
        entry.appendChild(createSpan("description", descriptionText));
        entry.appendChild(deleteButton(entry));

        container.appendChild(entry);
    }

    function createSpan(className, text) {
        var spn = document.createElement("span");
        spn.classList.add(className);
        spn.appendChild(document.createTextNode(text));
        return spn;
    }

    // the delete element can be make so:
    function deleteButton(toDelete) {
        var btn = createSpan("remove", "remove");
        var _listener = btn.addEventListener("click", function () {
            btn.removeEventListener("click", _listener);
            toDelete.remove();
        });

        return btn;
    }

    // here you bind your createEntry to the button
    var _listener = document.querySelector(buttonSelector)
        .addEventListener("click", function () {  
            createEntry("title text here", "description text here");
        });

    // the only public API needed is the remove listener
    // to avoid memory leak
    return {
        removeListener: function () {
            document.removeEventListener("click", _listener);
        }
    }
}

You can even make it shorter using modern Javascript syntax:

function MakeEntryModule(buttonSelector, containerSelector) {
    const container = document.querySelector(containerSelector);

    function createEntry(titleText, descriptionText) {
        const entry = document.createElement("div");
        entry.classList.add("entry");
        entry.appendChild(createSpan("title", titleText));
        entry.appendChild(createSpan("description", descriptionText));
        entry.appendChild(deleteButton(entry));

        container.appendChild(entry);
    }

    function createSpan(className, text) {
        const spn = document.createElement("span");
        spn.classList.add(className);
        spn.appendChild(document.createTextNode(text));
        return spn;
    }

    function deleteButton(toDelete) {
        const btn = createSpan("remove", "remove");
        const _listener = btn.addEventListener("click", () => {
            btn.removeEventListener("click", _listener);
            toDelete.remove();
        });

        return btn;
    }

    const _listener = document.querySelector(buttonSelector)
        .addEventListener("click", () =>  
            createEntry("title text here", "description text here"));

    return {
        removeListener: () =>
            document.removeEventListener("click", _listener)
    }
}

You should use const and let instead of var if possible, use var only if you have browser compatibility requirements.

About arrow functions, they are very compact and elegant, but use with carefull as they have a different behaviour compared with normal functions.

They are good for event handlers as they bind the declaration context, with the "correct" this object scope.

But they can drive to performance issues in some situations.

I get rid of the objects array, if you really need you can add it without problem.

\$\endgroup\$
  • \$\begingroup\$ very helpful tips here, thank you for your help! \$\endgroup\$ – user8758206 Feb 6 at 22:33

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.