London | 25-ITP-Sept | Samuel Tarawally | Sprint 2 | Book Library

[Tarawally]

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

• View list of books on page load
• Add books with title, author, pages, and read status
• Validate input and show alerts for missing fields
• Remove books from the list

[cjyuan]

Good start!

[cjyuan]

• Shouldn't myLibrary be populated with some data for users who do not have any data in their browser's local storage?

• The objects in the restored array are not instances of Book. A a result, myLibrary will end up with two kinds of objects: Generic objects and Book objects (added on line 42). Even though this is not a problem for this simple app, it is a better practice to consistently keep the same type of data in myLibrary.

[cjyuan]

Can you also address the comment I left on the code on lines 18-21 in the previous review?

[Tarawally]

To populate data for users who do not have any, and to restore plain data objects back into book objects, the following code snippet has been applied:

  const storedLibrary = localStorage.getItem("myLibrary");

  if (storedLibrary) {
    const rawData = JSON.parse(storedLibrary);
    // Rehydrates plain data back into Book objects
    myLibrary = rawData.map(
      (data) =>
        new Book(data.title, data.author, Number(data.pages), data.check)
    );
  } else {
    // Seeds data for new users
    myLibrary = [
      new Book("The Hobbit", "J.R.R. Tolkien", 295, false),
      new Book("1984", "George Orwell", 328, true),
      new Book("Robinson Crusoe", "Daniel Defoe", 252, true),
      new Book("The Old Man and the Sea", "Ernest Hemingway", 127, false),
    ];
    saveStorage();
  }

[cjyuan]

Well done.

Please note that if pages are consistently stored values of type number, we would not need the conversion Number(data.pages).

[Tarawally]

Here is my refactored code based on the feedback:

Bug Fixes:

• Fixed localStorage integration so books persist and display on page load
• Corrected author data mapping
• Fixed 'Read' status logic
• Repaired delete functionality

Code Quality Improvements:

• Input sanitisation with .trim() and validation
• XSS prevention using textContent
• Ternary operators for cleaner conditional logic
• User feedback alerts on delete
• Form auto-reset after submission
• Clear table rendering with .forEach()

[cjyuan]

On line 41, the value of pages can still be a string that represents an invalid number of pages.
Can you ensure the value of pages represents a valid number of pages.

[Tarawally]

To address this I've made the following changes:

  const pages = Number(pagesInput.value);

  // Validates input: checks for empty strings, non-numbers, or negative values
  if (!title || !author || isNaN(pages) || pages <= 0) {
    alert("Please enter valid book details. Pages must be a positive number.");
    return;
  }
  const book = new Book(title, author, pages, readCheckbox.checked);

[cjyuan]

Not all positive finite numbers are valid "number of pages" though. What other numbers should also be rejected?

[Tarawally]

float would be a problem so i guess this would be better

if (!title || !author || isNaN(pages) || pages <= 0 || !Number.isInteger(pages)) {}

[Tarawally]

this could also be shorten to

if (!title || !author || !Number.isInteger(pages) || pages <= 0) {}

[cjyuan]

Can you also address the comment I left on the code on lines 18-21 in the previous review?

[Tarawally]

I've updated populateStorage to seed the library with default data if localStorage is empty. To ensure consistency, I used Array.prototype.map() to convert the plain objects returned by JSON.parse() back into Book instances.

[Tarawally]

I implemented explicit type conversion for the page count using the Number() function. I also added a guard clause using isNaN() to ensure only valid, positive numbers are accepted.

[cjyuan]

Changes look good. I will mark this PR as complete first.

[cjyuan]

Well done.

Please note that if pages are consistently stored values of type number, we would not need the conversion Number(data.pages).