NW | 25-ITP-Sep | TzeMing Ho | Sprint 2 | book-library-debugging #326

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.

Sign up for GitHub

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

Jump to bottom

Open

TzeMingHo wants to merge 4 commits into CodeYourFuture:main from TzeMingHo:book-library-debugging

+67 −67

TzeMingHo commented Nov 3, 2025

Learners, PR Template

Self checklist

I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
My changes meet the requirements of the task
I have tested my changes
My changes follow the style guide

Changelist

I have debugged the book library, which now populates the book collection, displays book information correctly, adds books, and deletes books.


          completed book-library debugging

1b926cf

TzeMingHo added 📅 Sprint 2 Needs Review labels

Contributor

cjyuan commented Nov 12, 2025

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 and removed Needs Review labels

TzeMingHo added 2 commits

November 12, 2025 13:06


          After reading feedback, some improvement

3ccacab


          updated branch name

34fe6f0

Author

TzeMingHo commented Nov 12, 2025

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.

Thank you CJ,

The feedback document is very benefitical. I didn't notice that the project I sumbited still got so many bugs. Thank you for letting me know. I have made some improvements.

TzeMingHo added Needs Review and removed Reviewed labels

A-O-Emmanuel added Review in progress and removed Needs Review labels

A-O-Emmanuel reviewed

View reviewed changes

A-O-Emmanuel left a comment •

edited

Loading

Your book library works, I just used it and it seems okay, @cjyuan do you have any other comments so that I can mark it complete, thanks

A-O-Emmanuel added Reviewed and removed Review in progress labels

cjyuan reviewed

View reviewed changes

Contributor

cjyuan left a comment

Code works. I just have a few suggestions.

debugging/book-library/index.html Outdated

    
                  <div id="demo" class="collapse">

                    <div class="form-group">

                    <form class="form-group" id="book-form" onsubmit="handleSubmit(event)" >

Contributor

cjyuan Nov 12, 2025

Could consider attaching event handler to the form in JS (to keep HTML code free of JS code)

Author

TzeMingHo Nov 13, 2025

Yes. I have removed the function from the HTML and created an eventListener in the script.

debugging/book-library/script.js Outdated

    
                  pages.value == ""

                  !titleInput.value.trim() ||

                  !authorInput.value.trim() ||

                  pagesInput.value <= 0

Contributor

cjyuan Nov 12, 2025

Some "weird" numbers (that shouldn't be considered valid page count) could still pass this check. Can you figure out what these numbers are and "handle" them appropriately?

Author

TzeMingHo Nov 13, 2025

I tried my best to think of the weird numbers could be some spaces before or after the actual numbers. I added a trim process for the pages value as well. I hope this will help prevent some strange situations.

debugging/book-library/script.js Outdated

    
                } else {

                  let book = new Book(title.value, title.value, pages.value, check.checked);

                  library.push(book);

                  let book = new Book(String(titleInput.value), String(authorInput.value), Number(pagesInput.value), checkInput.checked);

Contributor

cjyuan Nov 12, 2025

.value is always a string. And on line 39, they could still contain leading or trailing spaces.

debugging/book-library/script.js Outdated

    
                for (let n = rowsNumber - 1; n > 0; n-- {

                  table.deleteRow(n);

                }

                let tableBody = document.getElementById("display-body")

Contributor

cjyuan Nov 12, 2025

Could consider (in code refactoring stage) change let to const for all variables that are not going to be reassigned a value.


          took the eventhandler off from html, and trim pagesValue

64e5908

TzeMingHo added Needs Review and removed Reviewed labels

cjyuan reviewed

View reviewed changes

Contributor

cjyuan left a comment

Changes look great!

debugging/book-library/script.js

Comment on lines 33 to 38

    
                let pagesValue = Number(pagesInput.value.trim());

                let read = checkInput.checked;

                if (

                  !titleInput.value.trim() ||

                  !authorInput.value.trim() ||

                  pagesInput.value <= 0

                  !titleValue||

                  !authorValue ||

                  !Number.isInteger(pagesValue) ||

Contributor

cjyuan Nov 13, 2025 •

edited

Loading

Trim of pagesinput.value is optional but ensuring pagesValue is an integer would prevent "weird" page count. So, well done!

debugging/book-library/script.js

Comment on lines +51 to +53

    
              const bookForm = document.getElementById("book-form")

              bookForm.addEventListener("submit", (event) => handleSubmit(event));

Contributor

cjyuan Nov 13, 2025 •

edited

Loading

It is better to group code that is to be executed once (line 52) in one place (to make locating them easier). For example, in the window.onload callback function (lines 3-5).

debugging/book-library/index.html

    
                  </table>

                  <script src="script.js"></script>

                  <script src="script.js" defer></script>

Contributor

cjyuan Nov 13, 2025

Suggestion: Consider loading script.js as an ES module to isolate its scope from the global context:

<script src="script.js" type="module"></script>

This ensures that variables, functions, and imports in script.js don’t leak into the global namespace, helps prevent naming conflicts, and enables the use of modern JavaScript features like import and export.

Also, with type="module", defer is automatically enforced.

Author

TzeMingHo Nov 15, 2025

Thank you. It is something useful. Good to know.

cjyuan added Complete and removed Needs Review labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete 📅 Sprint 2