London | 26-ITP-May | Khaliun Baatarkhuu | Sprint 2 | Book Library by khaliun-dev · Pull Request #535 · CodeYourFuture/Module-Data-Flows

khaliun-dev · 2026-08-11T12:48:47Z

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

Fixed books not rendering on page load
Fixed error when submitting a new book
Fixed author being saved as the title
Fixed broken delete buttons
Fixed incorrect read status
Added validation for missing author
Clear the form after successfully adding a book

Testing

Tested adding books
Tested missing fields
Tested read/unread status
Tested deleting books
Checked browser console for error

Questions

None.

Changed input types for title and author fields to 'text'.

cjyuan

Can you check if any of this general feedback can help you further improve your code?
https://github.com/CodeYourFuture/Module-Data-Flows/blob/general-review-feedback/debugging/book-library/feedback.md

Doing so can help me speed up the review process. Thanks.

khaliun-dev · 2026-08-12T11:23:01Z

@cjyuan i have made some improvements to the code from the general feedback, specifically:

Changes

Fixed book rendering on page load.
Fixed book submission and author handling.
Fixed Delete functionality.
Fixed Read/No status.
Added validation for missing fields.
Clear the form after adding a book.
Improved data types by storing pages as numbers.
Improved DOM variable naming.
Replaced innerHTML with textContent.
Removed unnecessary button IDs.
Simplified Read/No logic.
Used const where variables aren't reassigned.

Testing

Tested adding, validating, displaying, toggling Read/No, deleting books, and clearing the form. Browser console checked with no errors.

cjyuan · 2026-08-12T16:43:03Z

According to https://validator.w3.org/, there are errors in your index.html. Could you fix these errors?

cjyuan · 2026-08-12T16:47:14Z

Even though this element has a required attribute, the user could still "submit" the input the even if this input field is empty. Could you find out why and then address the issue?

If possible, also make this input field to accept only positive whole numbers.

cjyuan · 2026-08-12T16:50:22Z

The checkbox is not showing.

The issue is related to Bootstrap 4.4.1. Could you use AI to find a way to fix the issue? Mentioning "Bootstrap 4.4.1" might help.

cjyuan · 2026-08-12T16:52:25Z

          value="Submit"
          class="btn btn-primary"
-          onclick="submit();"
+          onclick="addBook();"


Could you research the trade-off between

Attaching event listener in HTML

Attaching event listener using addEventListener() in JS

cjyuan · 2026-08-12T16:52:53Z

Can we declare myLibrary in a way that prevents it from being accidentally reassigned?

cjyuan · 2026-08-12T16:56:54Z

+  const book = new Book(
+    titleInput.value,
+    authorInput.value,
+    Number(pagesInput.value),
+    readInput.checked
+  );


What if the input contains leading and trailing space characters?

Could Number(pagesInput.value) evaluates to an invalid page count? What numbers should be considered "invalid page count"?

Note: A better and safer approach to deal with user input is to first preprocess/sanitise them, and then store the cleaned values in variables. Thereafter, refer only to the variables for cleaned values consistently throughout the rest of the code.

cjyuan · 2026-08-12T16:59:03Z

+  titleInput.value = "";
+  authorInput.value = "";
+  pagesInput.value = "";
+  readInput.checked = false;


Note: If the input elements are inside a <form> element, we could also just call a form method to reset the form.

cjyuan · 2026-08-12T17:01:57Z

+  const rowsNumber = table.rows.length;
+
+
+  for (let n = rowsNumber - 1; n > 0; n--) {
    table.deleteRow(n);
  }


Clearing a table by deleting its rows row by row is not efficient. Could you look up a more efficient way to clear a table (the <tbody> part of the table)?

cjyuan · 2026-08-12T17:02:53Z

      alert(`You've deleted title: ${myLibrary[i].title}`);
      myLibrary.splice(i, 1);
      render();


The alert message is shown before the book is actually deleted; the deletion only occurs after the alert dialog is dismissed. This introduces a risk that the operation may not complete (e.g., if the user closes the browser before dismissing the alert).

In general, it’s better to display a confirmation message only after the associated operation has successfully completed.

alert() is a blocking function call. As a result, invoking it prevents the browser from updating the UI until the dialog is dismissed.

If time permits, research for approaches that allows the UI to update before displaying the alert dialog. (This is an optional change).

khaliun-dev · 2026-08-12T22:24:08Z

@cjyuan I have addressed all the comments you have made. Thanks.

cjyuan

Changes look good. Well done.

cjyuan · 2026-08-13T12:08:41Z

-    library.push(book);
-    render();
-  }
+const bookForm = document.getElementById("bookForm");


Could consider placing all code that runs once on page load in a single function. For example, you could put it inside the page load callback or create a function named init() or setup() and call it once when the page loads.

This makes it easier to locate and manage all the code that runs once when the app starts.

cjyuan · 2026-08-13T12:11:23Z

+const titleInput = document.getElementById("title");
+const authorInput = document.getElementById("author");
+const pagesInput = document.getElementById("pages");
+const readInput = document.getElementById("check");


Common practice is to declare all shared variables/constants at the beginning of the files, before function definitions. Doing so makes locating them easier.

cjyuan · 2026-08-13T12:11:51Z

+  !Number.isFinite(pages) ||
+  pages < 1 ||
+  !Number.isInteger(pages)


Could you figure out which check is redundant?

the code on line 43, due to another code preventing invalid values anyway.

cjyuan · 2026-08-13T12:12:42Z

      myLibrary.splice(i, 1);
      render();
+
+      alert(`You've deleted title: ${deletedTitle}`);


alert() is a blocking function call. As a result, invoking it prevents the browser from updating the UI until the dialog is dismissed.

If time permits, research for approaches that allows the UI to update before displaying the alert dialog. (This is an optional change).

Change alert to use setTimeout for better UX.

khaliun-dev added 3 commits August 10, 2026 22:36

Update input types for title and author fields

5d4442d

Changed input types for title and author fields to 'text'.

Fix book submission and rendering logic

7736b33

Fix book library bugs

cb6b3a7

khaliun-dev added 📅 Sprint 2 Assigned during Sprint 2 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Data-Flows The name of the module. labels Aug 11, 2026

cjyuan reviewed Aug 11, 2026

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 Aug 11, 2026

Improve book library code

6b83303

khaliun-dev added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 12, 2026

khaliun-dev mentioned this pull request Aug 12, 2026

London | 26-May-ITP | Data flows | Khaliun Baatarkhuu khaliun-dev/My-Coursework-Planner#25

Open

cjyuan reviewed Aug 12, 2026

View reviewed changes

cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 12, 2026

Address reviewer feedback

e7089ff

khaliun-dev added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 12, 2026

cjyuan reviewed Aug 13, 2026

View reviewed changes

Delay alert after book deletion

5316766

Change alert to use setTimeout for better UX.

Uh oh!

Uh oh!

Conversation

khaliun-dev commented Aug 11, 2026

Changelist

Testing

Questions

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

khaliun-dev commented Aug 12, 2026

Changes

Testing

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!

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!

khaliun-dev commented Aug 12, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants