Sheffield | 26-ITP-Jan | Mahmoud Shaabo | Sprint 2 | Book Library by mahmoudshaabo1984 · Pull Request #450 · CodeYourFuture/Module-Data-Flows

mahmoudshaabo1984 · 2026-04-16T18:07:44Z

What bugs did I fix?

Bug 1 - Website loads but doesn't show any books
The for loop in the render function was missing a closing parenthesis ) before the {. This caused a syntax error that stopped all JavaScript from running.
Bug 2 - Error in console when you try to add a book
The submit function was calling library.push(book) but the array is named myLibrary. Fixed by using the correct variable name. Also added validation for the author field.
Bug 3 - It uses the title name as the author name
In the submit function, title.value was used twice. The second argument should be author.value.
Bug 4 - Delete button is broken
The delete button variable was created as delButton but then referenced as delBut three times. Also the event name was "clicks" instead of "click". Fixed both issues.
Bug 5 - Read status saves the wrong answer
The if condition was inverted. check == false was showing "Yes". Fixed so check == true correctly shows "Yes".

How to test

Open index.html in the browser
Two books appear on load — both show "Yes" in the Read column
Click "Add new book" and fill in the form — the new book appears in the table
Click the "Yes"/"No" button on any book — it toggles correctly
Click "Delete" on any book — the book is removed from the table
Try submitting the form with empty fields — an alert appears

Hi @cifarquhar — I have fixed all 5 bugs listed in the readme. Please let me know if you have any feedback. Thank you!

mahmoudshaabo1984 · 2026-04-16T18:11:56Z

Hi @cifarquhar — PR #450 is ready for review.

I have fixed all 5 bugs listed in the readme.md file. The app now loads correctly, books can be added and deleted, and the read status toggles as expected.

Please let me know if anything needs to be changed. Thank you!

cjyuan

You have fixed all the bugs, but there are still plenty of room you can improve the code.

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.

mahmoudshaabo1984 · 2026-04-17T17:24:57Z

Hi CJ,

Thank you for your feedback. I have reviewed your comments and applied the following improvements:

script.js

Changed let to const for all DOM references that never change
Renamed confusing variables: check → hasBeenRead, title → titleInput, author → authorInput
Renamed submit() to addBook() to avoid conflict with the browser's built-in submit behaviour
Replaced innerHTML with textContent for all user-provided data to prevent XSS vulnerabilities
Renamed populateStorage() to addDefaultBooks() — the old name implied localStorage usage
Extracted createReadButton() and createDeleteButton() as separate helper functions to make render() easier to read
Added form field clearing after a book is successfully added

index.html

Added lang="en" to the <html> element for accessibility
Added a proper page title: "My Book Library"
Fixed invalid input types: type="title" and type="author" changed to type="text"

Please let me know if there is anything else to improve.

Thank you,
Mahmoud Shaabo

cjyuan · 2026-04-18T03:25:03Z

@@ -1,103 +1,136 @@
+// myLibrary is the main array — like a shelf that holds all book objects
 let myLibrary = [];


Safer to declare myLibrary using const because it is not supposed to be reassigned a value.

cjyuan · 2026-04-18T03:25:33Z

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


Good practice to declare all shared constants/variables at the beginning of the file.

cjyuan · 2026-04-18T03:29:52Z

+    myLibrary.push(new Book("Robinson Crusoe", "Daniel Defoe", "252", true));
+    myLibrary.push(
+      new Book("The Old Man and the Sea", "Ernest Hemingway", "127", true)
    );


It is better to use an appropriate data type to represent all data.
Could consider storing "page count" as a value of type "number".

cjyuan · 2026-04-18T03:48:56Z

+    titleInput.value === "" ||
+    authorInput.value === "" ||
+    pagesInput.value === ""


String input may contain only space characters or leading/trailing space. Ideally the app should not accept a string containing only spaces as a book title or author's name.

pageInput.value is a string and it can be a number in scientific format (e.g. "3e2") -- which may look weird when displayed as a value of page count.

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-04-18T03:56:10Z

+  button.addEventListener("click", function () {
+    // splice(index, 1) removes exactly 1 item at position "index"
+    myLibrary.splice(index, 1);
+    render();
+  });


The original code has a statement that uses window.alert() to show the title of the deleted book whenever a book is deleted.

cjyuan · 2026-04-18T03:58:30Z

+function createReadButton(index) {
+  const button = document.createElement("button");
+  button.className = "btn btn-success";
+
+  // Use textContent instead of innerHTML for safety
+  // textContent never runs any HTML or scripts — innerHTML can be dangerous
+  // if the text comes from user input
+  button.textContent = myLibrary[index].hasBeenRead ? "Yes" : "No";
+
+  button.addEventListener("click", function () {
+    // Flip the read status: true becomes false, false becomes true
+    myLibrary[index].hasBeenRead = !myLibrary[index].hasBeenRead;
+    render();
+  });
+
+  return button;
+}


I like the way you use function to isolate the logic for creating the the Read and Delete buttons.

cjyuan · 2026-04-18T03:59:29Z

+  const table = document.getElementById("display");
+  const rowCount = table.rows.length;
+
+  // Delete all rows except the header row (row 0)
+  for (let i = rowCount - 1; i > 0; i--) {
+    table.deleteRow(i);
  }


For better performance (and also simpler code), you can consider clearing <tbody> instead.

fix: resolve all 5 bugs in book-library

bcb28b1

This comment has been minimized.

Sign in to view

mahmoudshaabo1984 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 16, 2026

This comment has been minimized.

Sign in to view

github-actions bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 16, 2026

mahmoudshaabo1984 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 16, 2026

This comment has been minimized.

Sign in to view

github-actions bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 16, 2026

mahmoudshaabo1984 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 16, 2026

cjyuan reviewed Apr 17, 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 Apr 17, 2026

refactor: improve code quality based on CJ feedback

23f8830

mahmoudshaabo1984 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 17, 2026

cjyuan reviewed Apr 18, 2026

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Sheffield | 26-ITP-Jan | Mahmoud Shaabo | Sprint 2 | Book Library#450

Sheffield | 26-ITP-Jan | Mahmoud Shaabo | Sprint 2 | Book Library#450
mahmoudshaabo1984 wants to merge 2 commits intoCodeYourFuture:mainfrom
mahmoudshaabo1984:book-library-mahmoud

mahmoudshaabo1984 commented Apr 16, 2026

Uh oh!

This comment has been minimized.

mahmoudshaabo1984 commented Apr 16, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

cjyuan left a comment

Uh oh!

mahmoudshaabo1984 commented Apr 17, 2026

Uh oh!

cjyuan Apr 18, 2026

Uh oh!

cjyuan Apr 18, 2026

Uh oh!

cjyuan Apr 18, 2026

Uh oh!

cjyuan Apr 18, 2026

Uh oh!

cjyuan Apr 18, 2026

Uh oh!

cjyuan Apr 18, 2026

Uh oh!

cjyuan Apr 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -1,103 +1,136 @@
		// myLibrary is the main array — like a shelf that holds all book objects
		let myLibrary = [];

Uh oh!

Conversation

mahmoudshaabo1984 commented Apr 16, 2026

What bugs did I fix?

How to test

Uh oh!

This comment has been minimized.

mahmoudshaabo1984 commented Apr 16, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

mahmoudshaabo1984 commented Apr 17, 2026

Uh oh!

cjyuan Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants