Todo list programming best practice

[bushmusi]

Hi @carlylechia,

Status: Required Changes :recycle:

You have implemented below features well :clap:

• [x] No linter error
• [x] Professional Readme
• [x] Meaningful commit message
• [ ] Followed GitFlow
• [x] Followed HTML, CSS, and JS best practice

Recommended Changes

• [ ] I kindly suggest you use forEach method instead of for loop here because it will improve your code readability. Instead you can use const resetIndex = (tasks) => { tasks.forEach( (val,index) => val.index = index + 1 };

• [ ] I recommend you to use one line conditional statement here and I don't think you need to compare tsk.checked ===true because tsk.checked will only return boolean. tsk.checked ? checkbox.setAttribute('checked', 'checked') : null;

---

Cheers and Happy coding!👏👏👏

Feel free to leave any questions or comments in the PR thread if something is not 100% clear. Please, remember to tag me in your question so I can receive the notification.

Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.

[carlylechia]

Thank you so much, Bushra. I'll make the changes you've requested and let you know

On Mon, Aug 8, 2022 at 8:00 AM Bushra Mustofa @.***> wrote:

Hi @carlylechia https://github.com/carlylechia, Status: Required Changes ♻️ You have implemented below features well 👏

• No linter error
• Professional Readme
• Professional commit message and PR
• Followed HTML, CSS, and JS best practice

Recommended Changes

-

I kindly suggest you use forEach method instead of for loop here https://github.com/carlylechia/to-do-list-review/blob/0f8e443f5e6928b193eb978e1d367af7e616f331/index.js#L21-L27 because it will improve your code readability. Instead you can use const resetIndex = (tasks) => { tasks.forEach( (val,index) => val.index = index + 1 };

I recommend you to use one line conditional statement here https://github.com/carlylechia/to-do-list-review/blob/0f8e443f5e6928b193eb978e1d367af7e616f331/index.js#L45and I don't think you need to compare tsk.checked ===true because tsk.checked will only return boolean. tsk.checked ? checkbox.setAttribute('checked', 'checked') : null;

— Reply to this email directly, view it on GitHub https://github.com/carlylechia/to-do-list-review/issues/2, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOW4WVG5RJKJYHFCFMNYSQTVYCWCBANCNFSM5PTLMAZA . You are receiving this because you were mentioned.Message ID: @.***>