Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do I follow JavaScript best practices? -- Self code review. #4

Open
Kakalanp opened this issue Feb 21, 2022 · 0 comments
Open

Do I follow JavaScript best practices? -- Self code review. #4

Kakalanp opened this issue Feb 21, 2022 · 0 comments

Comments

@Kakalanp
Copy link
Owner

DRY rule:

  • In these parts, I declared the dynamic event listeners separately each time createList is executed:

    TO-DO-list/src/index.js

    Lines 46 to 48 in e84dbd5

    createList();
    addTask.value = '';
    deleteSingleTask();

    TO-DO-list/src/index.js

    Lines 60 to 63 in e84dbd5

    document.addEventListener('DOMContentLoaded', () => {
    createList();
    deleteSingleTask();
    });

    TO-DO-list/src/index.js

    Lines 34 to 38 in e84dbd5

    deleteList = [];
    createList();
    deleteSingleTask();
    }
    Although this is not very notorious, it may lead to some loops and exponentially growing code whenever the first function is called, a great solution to the problem would be fixing the next method:

    TO-DO-list/src/index.js

    Lines 9 to 21 in e84dbd5

    function deleteSingleTask() {
    const deleteBtns = document.getElementsByClassName('delete-button');
    const deleteBtnsArr = Array.from(deleteBtns);
    deleteBtnsArr.forEach((el, i) => {
    el.addEventListener('click', () => {
    const deleteList = [];
    deleteList.push(i);
    deleteTask(deleteList);
    el.parentElement.parentElement.remove();
    });
    });
    }
    A great solution to the issue would be to, first, move the event listeners to the end of the createList declaration

    TO-DO-list/src/index.js

    Lines 10 to 14 in e84dbd5

    const deleteBtns = document.getElementsByClassName('delete-button');
    const deleteBtnsArr = Array.from(deleteBtns);
    deleteBtnsArr.forEach((el, i) => {
    el.addEventListener('click', () => {
    followed by the second step, importing the algorithms from another file, and calling them as a function inside the event listeners moved beforehand

    TO-DO-list/src/index.js

    Lines 15 to 18 in e84dbd5

    const deleteList = [];
    deleteList.push(i);
    deleteTask(deleteList);
    el.parentElement.parentElement.remove();
    This way, everything is more organized and functional, preventing bugs and reducing the propensity to elongate the code meaninglessly.

HTML best practices:

  • You can always add your h1 tag inside a header tag whenever you don't have a navbar:
    <h1 class="title">Today's To Do</h1>
    <input type="text" id="addText" placeholder="Add to your list...">
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant