London | 26-ITP-May | Boualem Larbi Djebbour | Sprint 2 | Book Library #498
London | 26-ITP-May | Boualem Larbi Djebbour | Sprint 2 | Book Library #498djebsoft wants to merge 13 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
djebsoft
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
djebsoft
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
djebsoft
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
djebsoft
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
There are still some improvements you can make.
Can you first check if any of this general feedback can help you further improve your code?
https://github.com/CodeYourFuture/Module-Data-Flows/blob/general-review-feedback/debugging/book-library/feedback.md
Doing so can help me speed up the review process. Thanks.
cjyuan
added
Reviewed
djebsoft
added
Needs Review
There was a problem hiding this comment.
Can we declare myLibrary in a way that prevents it from being accidentally reassigned?
| const titleInput = document.getElementById("title"); | ||
| const authorInput = document.getElementById("author"); | ||
| const pagesInput = document.getElementById("pages"); | ||
| const checkInput = document.getElementById("check"); |
There was a problem hiding this comment.
Could consider moving these declaration to the top. Common practice is to declare all shared (global) variables/constants at the begging of the file, before function definition.
| function populateStorage() { | ||
| if (myLibrary.length == 0) { | ||
| let book1 = new Book("Robison Crusoe", "Daniel Defoe", "252", true); | ||
| let book1 = new Book("Robinson Crusoe", "Daniel Defoe", "252", true); |
There was a problem hiding this comment.
Better practice is to consistently represent the data using the same data type. Here, page count is a string, and on line 50, page count is a value of type number.
| for (let n = rowsNumber - 1; n > 0; n-- { | ||
| table.deleteRow(n); | ||
| } | ||
| let tbody = table.tBodies[0]; |
There was a problem hiding this comment.
After ensuring the code is working correctly, a good practice during the refactoring phase is to change let to const for variables that will not be reassigned. Some of the variables fit this characteristic.
| alert(`You've deleted title: ${myLibrary[i].title}`); | ||
| myLibrary.splice(i, 1); | ||
| render(); |
There was a problem hiding this comment.
The alert message is shown before the book is actually deleted; the deletion only occurs after the alert dialog is dismissed. This introduces a risk that the operation may not complete (e.g., if the user closes the browser before dismissing the alert).
In general, it’s better to display a confirmation message only after the associated operation has successfully completed.
cjyuan
removed
the
Needs Review
cjyuan
added
the
Reviewed
djebsoft
added
Needs Review
2 participants
Learners, PR Template
Self checklist
Changelist
This PR previously discussed in #482
changes since last PR:
-responded to the reviewer comments and fixed bugs