Chimoneg27/To-do-list-review

Peer to peer code review

Opened this issue · 2 comments

Peer to peer code review

Hi @Chimoneg27 ,

Your project is looking great!

To Highlights

✔️ Descriptive commit messages
✔️ Professional** looking README file
✔️ Good overall design
✔️ Good use of GitHub flows 👍
✔️ Overall, the code seems well-organized and follows JavaScript best practices. It separates concerns, uses functions for modularization, and efficiently updates the UI and local storage when interacting with todos.

Suggestion

There's a potential issue with reassigning the todos array in the clearCompletedBtn event listener.
clearCompletedBtn.addEventListener('click', () => {
todos = todos.filter((todo) => !todo.completed);

todos.forEach((todo, index) => {
todo.id = index;
});

renderTodos();
localStorage.setItem('todos', JSON.stringify(todos));
});
It is not good practice to reassign a variable declared with let outside its scope. Instead, use methods like splice or create a new array with filtered values. The same goes for the event listener that handles checkbox changes.

Cheers and Happy coding!👏👏👏

Hi @Chimoneg27 ,

Your project is looking great!

To Highlights
✔️ Descriptive commit messages
✔️ Professional** looking README file
✔️ Good overall design
✔️ Good use of GitHub flows 👍
✔️ Overall, the code seems well-organized and follows JavaScript best practices. It separates concerns, uses functions for modularization, and efficiently updates the UI and local storage when interacting with todos.

Suggestion

  • There's a potential issue with reassigning the todos array in the clearCompletedBtn event listener.
    clearCompletedBtn.addEventListener('click', () => {
    todos = todos.filter((todo) => !todo.completed);

todos.forEach((todo, index) => {
todo.id = index;
});

renderTodos();
localStorage.setItem('todos', JSON.stringify(todos));
});

  • In the line 63, We see 'return undefined' which doesn't suit Javascript best practices. We suggest to change "undefined" => " ".
    It is not good practice to reassign a variable declared with let outside its scope. Instead, use methods like splice or create a new array with filtered values. The same goes for the event listener that handles checkbox changes.

Cheers and Happy coding!👏👏👏