Sheffield | May 2025 | Hassan Osman | Sprint 2 | Book Library Project | Feature/book library#279
Sheffield | May 2025 | Hassan Osman | Sprint 2 | Book Library Project | Feature/book library#279HassanOHOsman wants to merge 19 commits intoCodeYourFuture:mainfrom
Conversation
HassanOHOsman
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
cjyuan
added
Reviewed
HassanOHOsman
added
Needs Review
Thanks CJ. I've updated my files with the suggested work accordingly. |
| if (isNaN(pagesInput.value) || pagesInput.value <= 0 || pagesInput.value > 10000) { | ||
| alert("Please enter a valid page count (1–10000)."); | ||
| return false; | ||
| } |
| titleInput.value = titleInput.value.trim().replace(/\s+/g, " "); | ||
| authorInput.value = authorInput.value.trim().replace(/\s+/g, " "); | ||
| pagesInput.value = pagesInput.value.trim(); | ||
|
|
There was a problem hiding this comment.
Why not introduce variables with shorter names so that you don't have to type as much in the code?
There was a problem hiding this comment.
Are the variable names that long? 😅 For example, I could've changed "titleInput" to "titleInpt" or "titInput" but I didn't want to compromise on the meaningfulness of the variables. Having said that, I'll aim to produce as much shorter & meaningful variable names in the future moving forward. Thank you CJ 😁
debugging/book-library/script.js
Outdated
| let readStatus = ""; | ||
| if (myLibrary[i].check == false) { | ||
| readStatus = "Yes"; | ||
| } else { | ||
| readStatus = "No"; | ||
| } else { | ||
| readStatus = "Yes"; | ||
| } | ||
| changeBut.innerText = readStatus; | ||
| readBtn.innerText = readStatus; |
There was a problem hiding this comment.
This could be a good opportunity to practice using the ? : conditional operator. Can you rewrite the code on lines 92-98 as a single statement?
cjyuan
added
Complete
|
Your PR's title isn't in the expected format. Please check its title is in the correct format, and update it. Reason: Wrong number of parts separated by |s |
3 participants
Learners, PR Template
Self checklist
Changelist
Implemented a book list app with features to add, view, and delete books, including title, author, page count, and read status.
Fixed bugs related to incorrect author assignment, broken delete button, read status display, and form validation.
Questions
Ask any questions you have for your reviewer.