London | 25-ITP-Sep| Shaghayegh Shirinfar | Sprint 2 | Book library #340
Conversation
shaghayeghfar
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Can you check if any of this general feedback can help you further improve your code? https://github.com/cjyuan/Module-Data-Flows/blob/book-library-feedback/debugging/book-library/feedback.md
Doing so can help me speed up the review process. Thanks.
cjyuan
added
Reviewed
|
Dear @cjyuan thank you for reviewing my code, I did some changes to my code. |
shaghayeghfar
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Everything looks great so far! I only have a couple of small follow-ups.
debugging/book-library/script.js
Outdated
| const pages = pagesInput.value; | ||
|
|
||
| // FEEDBACK APPLIED: improved validation and normalization | ||
| if (!title || !author || !pages || Number(pages) <= 0) { |
There was a problem hiding this comment.
A lot of numbers that should probably be treated as invalid page counts could pass this check.
Can you reject or post-process these numbers?
There was a problem hiding this comment.
thanks again for your feedback
I improved the page number check so it only lets real, positive whole numbers through. It now removes extra spaces, makes sure the value is actually a number, doesn’t allow decimals or negative numbers, and also blocks extremely large page counts.
debugging/book-library/script.js
Outdated
| clearForm(); | ||
|
|
||
| // Collapse form | ||
| $("#addForm").collapse("hide"); |
There was a problem hiding this comment.
Why use mixed syntax in manipulating DOM? It is generally not commended for new project.
Note: Do you recognize what 3rd party dependency is needed in order for this statement to work?
There was a problem hiding this comment.
thank you.
I replaced the jQuery collapse line with the Bootstrap 5 JavaScript API, so the form now closes using only plain JavaScript. This removes the jQuery dependency and keeps the code consistent with the rest of the project.
const collapse = bootstrap.Collapse.getOrCreateInstance(document.getElementById("addForm"));
collapse.hide();
debugging/book-library/script.js
Outdated
| $("#addForm").collapse("hide"); | ||
| } | ||
|
|
||
| addBtn.addEventListener("click", submitBook); |
There was a problem hiding this comment.
Putting the code on line 84 and the code on lines 28-29 (code that is executed once on page load) in one place could make it easier to understand what’s happening when the page loads.
There was a problem hiding this comment.
thank you . I moved all the code that runs when the page loads (loading the library, rendering the table, and setting up the add button) into one DOMContentLoaded block. This makes it easier to see what happens on page load.
cjyuan
added
Reviewed
shaghayeghfar
added
Needs Review
| !Number.isInteger(pagesNum) || | ||
| pagesNum <= 0 || | ||
| pagesNum > 10000 || // reject unrealistic values | ||
| !isFinite(pagesNum) // rejects Infinity and NaN |
There was a problem hiding this comment.
Some of the checks on line 67, 68, 71 are redundant. Can you figure out which?
cjyuan
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
I fixed the errors and fixed the add button.