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: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.

mahmoudshaabo1984 · 2026-04-18T10:26:12Z

Hi CJ,

Thank you for the detailed second round of feedback. I have applied all of your suggestions:

Changed let myLibrary to const myLibrary — it is never reassigned, only mutated.
Changed page count from a string ("252") to a number (252) — correct data type for numeric data.
Added input sanitisation before validation:
- sanitiseText() trims leading and trailing whitespace, preventing inputs with only spaces from being accepted.
- sanitisePages() rejects scientific notation like "3e2" and converts the value to a safe integer using Math.floor().
Restored the alert() that shows the title of a deleted book when the Delete button is clicked.
Replaced the row-by-row deletion loop with tbody.innerHTML = "" — simpler and more performant.

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

Thank you,
Mahmoud Shaabo

cjyuan

Changes look good. Well done.

cjyuan · 2026-04-18T10:35:08Z

+    alert(`"${myLibrary[index].title}" has been deleted.`);
+    myLibrary.splice(index, 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.

cjyuan · 2026-04-18T10:41:22Z

+  if (trimmed.toLowerCase().includes("e")) {
+    return NaN;
  }


Can also just let Number() convert the input even if it contains an 'e' -- less code to maintain.

illicitonion · 2026-05-04T16:57:15Z

Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it.

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

refactor: apply CJ second round feedback

1118739

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

cjyuan reviewed Apr 18, 2026

View reviewed changes

mahmoudshaabo1984 mentioned this pull request Apr 23, 2026

Sheffield 26-ITP-Jan | Data flows | Mahmoud Shaabo mahmoudshaabo1984/My-Coursework-Planner#35

Open

illicitonion closed this May 4, 2026

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

cjyuan Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mahmoudshaabo1984 commented Apr 18, 2026

Uh oh!

cjyuan left a comment

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!

illicitonion commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants