West Midlands | 25-ITP-Sep | Baba Yusuf | Sprint 2 | Feature/book library#358
West Midlands | 25-ITP-Sep | Baba Yusuf | Sprint 2 | Feature/book library#358Baba05206 wants to merge 9 commits intoCodeYourFuture:mainfrom
Conversation
Baba05206
added
📅 Sprint 2
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
…, Book class; add unit tests
|
I have implemented the improvements. I did make a few mistakes in the process that i tried to reverse, but could not be sure i reversed all. But requested improvements were implemented. |
Baba05206
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
What you have is a rewritten and enhanced version of the original app!
Code looks good. Just have a few questions to check you understanding.
debugging/book-library/script.js
Outdated
| } else { | ||
| readStatus = "No"; | ||
|
|
||
| if (!pagesRaw || !Number.isFinite(pages) || pages <= 0) { |
There was a problem hiding this comment.
One of the checks is redundant.
There was a problem hiding this comment.
Thanks for pointing that out. I see that I was checking/converting the same value twice. I’ve removed the extra check to keep the code cleaner
debugging/book-library/script.js
Outdated
| const parsed = JSON.parse(raw); | ||
| if (!Array.isArray(parsed)) return false; | ||
| myLibrary = parsed.map( | ||
| (o) => new Book(o.title, o.author, Number(o.pages), !!o.check) |
There was a problem hiding this comment.
Why do you need to explicitly type-cast o.pages and o.check?
There was a problem hiding this comment.
Thank you for the feedback! I realized that since my Book class constructor already handles the type conversion (using Number() and Boolean()), explicitly casting them again inside the .map() function in loadStorage was unnecessary and repetitive. I'll clean that up to make the code more concise.
debugging/book-library/script.js
Outdated
| function showConfirm(message) { | ||
| return new Promise((resolve) => { | ||
| confirmDescEl.textContent = message; | ||
| confirmModalEl.classList.remove("modal-hidden"); |
There was a problem hiding this comment.
Why implement this function to return a promise instead of true/false directly?
There was a problem hiding this comment.
I used a Promise because I wanted the code to 'wait' for the user to click a button in my custom modal, similar to how an API call works. However, I realize now that since this is a simple UI interaction, using a Promise adds unnecessary complexity. I will simplify the logic by moving the 'Delete' action directly into the button's click event handler instead of wrapping the whole process in a Promise
There was a problem hiding this comment.
I was mainly checking your understanding and you explained it well why you needed to use Promise.
The code looks correct to me and you didn't need to revert it.
cjyuan
removed
the
Needs Review
…prove accessibility
Baba05206
added
the
Needs Review
|
Well done in this exercise! Excellent implementation. |
cjyuan
added
Complete
3 participants
Learners, PR Template
Self checklist
Changelist
Fixed the issues with the 'My Book Library' website issues as per the readme instructions.
Questions