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

DRY, KISS, YAGNY check #5

Open
Gardimy opened this issue May 15, 2023 · 0 comments
Open

DRY, KISS, YAGNY check #5

Gardimy opened this issue May 15, 2023 · 0 comments

Comments

@Gardimy
Copy link
Owner

Gardimy commented May 15, 2023

Hi Gardimy,
Good job so far,😎
After reviewing your code, here are some potential issues and suggestions for improvement:

  • Unused import statement: The code imports addTask from './app.js', but it is not used anywhere in the code. If it's not needed, you can remove the import statement.
  • Function names and organization: The code mixes different functionalities within the populateTodoList function. It would be beneficial to split the code into smaller functions, each responsible for a specific task (e.g., rendering tasks, handling task updates, and event listeners). This helps improve code readability, maintainability and adheres to the Single Responsibility Principle.
  • Direct DOM manipulation: Instead of directly manipulating the innerHTML property of elements like a to-do list, consider using more structured approaches like creating elements using document.createElement and appending them to the DOM. This allows for better control over the HTML structure and helps prevent security vulnerabilities like cross-site scripting (XSS).
  • Redundant code: The updateLocalStorage function is called multiple times within the same function, even though it performs the same operation. It can be called once, at the end of the populateTodoList function, after all the changes have been made.
  • Checkbox event listener: The event listener for the checkbox changes the completed property of the task directly. It would be safer to create a copy of the task object and modify the copy instead of directly modifying the original task. This can help prevent unintended side effects and make the code more robust.
  • Inconsistent naming conventions: The code uses a mix of different naming conventions for CSS classes and JavaScript variables. It's generally recommended to use a consistent naming convention, such as camelCase or kebab-case for JavaScript and kebab-case for CSS classes.
  • Clear button functionality: The clear button event listener calls clearCompletedTasks and then immediately calls updateLocalStorage and populateTodoList. Since clearCompletedTasks already updates the localStorage and repopulates the todo list, these redundant calls can be removed.
  • Lack of error handling: The code assumes that the localStorage operations (getItem and setItem) will always succeed and that the stored data is always valid. It's good practice to include appropriate error handling, such as checking for errors when accessing local storage or parsing JSON data.
  • Lack of validation: There is no validation for the taskId passed to removeTask and data-id attribute of the remove button. It would be wise to validate the taskId and ensure it's a valid index before performing any operations with it.
  • Potential memory leak: The code attaches event listeners to the remove buttons using addEventListener inside the populateTodoList function. When the populateTodoList function is called repeatedly, new event listeners are added to the remove buttons without removing the old ones. This can lead to memory leaks over time. Consider removing existing event listeners before adding new ones.
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