London | 25-ITP-May | Houssam Lahlah | Sprint 2 | Feature/book library #322

HoussamLh · 2025-09-07T14:39:29Z

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

This pull request for syn my fork and updating my branch.

Questions

Please, Let me know if there is anything else went wrong?

- change library.push(book) to myLibrary.push(book). - add a ) to the end of the for loop condition to avoid syntax error. - change myLibrary[i].check to myLibrary[i].check == true to check if the book is read or not. - change delBut to delButton because we declare it. - The event listener is "clicks" instead of "click".

cjyuan

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.

…jects empty or invalid inputs

cjyuan · 2025-09-15T12:49:56Z

debugging/book-library/script.js

+  const pagesVal = Number(pagesEl.value);
+
+  if (!titleVal || !authorVal || !pagesVal) {
    alert("Please fill all fields!");
-    return false;
-  } else {
-    let book = new Book(title.value, title.value, pages.value, check.checked);
-    library.push(book);
-    render();
+    return;
  }
+
+  const book = new Book(titleVal, authorVal, pagesVal, checkEl.checked);


Input could be any kind of numbers but some of them should not be considered a valid page count.
Can you add code to sanitize or reject these invalid value?

Hi cjyuan,
Thank you for your guidence. you absoloutly right, I've add another if condition to handle the page number only and now submitBook() function validate the page count input.

It now rejects invalid values such as 0, negative numbers, non-numeric input, or decimals.

Only positive whole numbers are accepted as valid page counts.

Users will see a clear alert message when their input doesn’t meet these rules.

Have you tested the values which you said the app would reject? The modified code does not quite function the way you described here.

Hi cjyuan,

Thanks for catching that — I realized the behavior I described earlier didn’t fully match what was in the code (my mistake!).

I’ve now updated the validation to use a regex (/^\d+$/) instead of the simple Number() check. This ensures only positive whole numbers are accepted.

I tested with "0", "-5", and "12.5" — all of these are now correctly rejected, while "123" works as expected. Also, non-numeric characters like "abc" can’t even be typed into the input field anymore, so they’re automatically prevented at the keyboard level.

This should now fully match the intended behavior.

cjyuan · 2025-09-15T12:52:07Z

debugging/book-library/script.js

-      myLibrary.splice(i, 1);
-      render();
+    // Delete button
+    const delCell = row.insertCell(4);


Can you think of the pros and cons of these two approaches for creating cells within a row?

Keeping all the cell creation code in one location, like the original code does.

Scattering the cell creation code across different locations, like what you did.

Hi cjyuan,

Thanks for the feedback! I see the pros and cons:

Keeping all cell creation code in one place makes the table structure clearer.

Splitting it up makes the logic around buttons easier to manage but reduces readability.

To balance both, I’ll keep the cell creation in one centralized block but move the button setup (delete, toggle read) into small helper function. That way, the row structure stays easy to follow while keeping the button logic modular.

github-actions · 2025-09-15T12:53:34Z

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

…e button setup

cjyuan · 2025-09-18T11:18:06Z

debugging/book-library/script.js

  const table = document.getElementById("display");
-  const tbody = table.querySelector("tbody");
-  // clear old rows
-  tbody.innerHTML = ""; 
+  table.innerHTML = "";


Clearing the whole table would also remove the header row in the table.

Hi cjyuan,

Thanks for pointing this out! I updated my code to only clear the instead of the entire table, so the header row is preserved while old book rows are removed.

cjyuan · 2025-09-18T11:20:36Z

debugging/book-library/script.js

+    checkCell.textContent = book.check ? "Read" : "Not Read";
+


What about the checkbox event handler?

Hi cjyuan,

Thank you for your guidence.

I added an event handler for the Read/Not Read button, which toggles the check property on the correct book object and re-renders the table. This ensures the status updates immediately when clicked.

…e table

cjyuan

You probably missed my comment regarding the page count input.

Can you address this too? I will mark this PR as "Complete" first.

cjyuan · 2025-09-19T02:01:05Z

debugging/book-library/script.js

+function createDeleteCell(cell, book) {
  const deleteBtn = document.createElement("button");
  deleteBtn.textContent = "Delete";
  deleteBtn.addEventListener("click", () => {
+    const index = myLibrary.indexOf(book);
+    if (index > -1){
    myLibrary.splice(index, 1);
    render();
+    }
  });
  cell.appendChild(deleteBtn);
-}
-


Why use book instead of index? Both work for this simple app, I am just curious about your rationale.

Hi cjyuan,

Thank you for your assistance.
That’s a good question. I went with passing the book object instead of the index mainly for readability — it felt more natural to delete by reference since I already had access to the book when creating the cell.

I realize using index might be a bit more direct (and avoids calling indexOf), so I see the pros and cons of both approaches. For this small app both solutions work fine, but in a larger app with potential duplicates or performance concerns, using index would likely be the better option.

cjyuan · 2025-09-19T02:03:28Z

debugging/book-library/script.js

+    const readBtn = document.createElement("button");
+    readBtn.textContent = book.check ? "Read" : "Not Read";
+    readBtn.className = book.check ? "btn btn-success" : "btn btn-secondary";
+    readBtn.addEventListener("click", () => {
+      book.check = !book.check; // toggle
+      render(); // re-render table
+    });
+    checkCell.appendChild(readBtn);


Why implement a function for "delete button" but not doing the same consistently for the "check box"?

Hi cjyuan,

Thanks for the feedback!

You’re absolutely right. I know how important refactoring code is. We should always break down our functions into smaller functions that each do a specific job. This method helps make the code clean, reusable, and easier to understand. I’m sorry—I think I was in a rush this time. My apologies.

I refactored the read/unread button logic into a helper function similar to the delete button.

HoussamLh and others added 3 commits August 18, 2025 16:17

Merge branch 'CodeYourFuture:main' into feature/book-library

bcdfde3

Merge branch 'CodeYourFuture:main' into feature/book-library

fff1b69

HoussamLh added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Sep 7, 2025

cjyuan reviewed Sep 7, 2025

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Sep 7, 2025

HoussamLh mentioned this pull request Sep 4, 2025

London ITP May 25 | Data flows | Houssam Lahlah HoussamLh/My-Coursework-Planner#52

Open

HoussamLh added 8 commits September 15, 2025 00:17

Validate my HTML code online using W3.org

f439e87

Remove calling render() function to avoid duplicate

2537f48

Fix the page number to passe and input as a number not as a string

fd3649e

add a suffix like El and rename them to indicate DOM elements

94f7e3c

Removes leading/trailing spaces and converts pages to a number and re…

b5a0f4b

…jects empty or invalid inputs

Update to remove all rows at once and use innerText instead innerHTML

b0c6627

Renamed submit() to submitBook() to avoid HTML form conflicts

283fda2

fix: correct Book Library JS functionality

b2a7d5e

HoussamLh added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Sep 15, 2025

cjyuan reviewed Sep 15, 2025

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Sep 15, 2025

HoussamLh added 2 commits September 18, 2025 10:59

add validation for page count input in Book Library

9adee93

Refactor table row creation to centralize cell creation and modulariz…

b16c7f2

…e button setup

HoussamLh added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Sep 18, 2025

cjyuan reviewed Sep 18, 2025

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Sep 18, 2025

update the render() function to delete just the the rows not the whol…

1de7803

…e table

update the code to handle the toggle checkbox Read or Not Read

9b6df60

HoussamLh added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Sep 18, 2025

cjyuan reviewed Sep 19, 2025

View reviewed changes

cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Sep 19, 2025

HoussamLh added 3 commits September 19, 2025 13:05

validate pages input to accept only positive whole numbers

e953bb4

Refactor delete button to use index instead of book object

25f7083

Refactor read/unread button creation to use a helper function

e5f6ec3

Uh oh!

London | 25-ITP-May | Houssam Lahlah | Sprint 2 | Feature/book library #322

Are you sure you want to change the base?

London | 25-ITP-May | Houssam Lahlah | Sprint 2 | Feature/book library #322

Uh oh!

Conversation

HoussamLh commented Sep 7, 2025

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!