From bcb28b1c137aef866e38897fb27b336befe82c6c Mon Sep 17 00:00:00 2001 From: Mahmoud Shaabo Date: Thu, 16 Apr 2026 18:58:40 +0100 Subject: [PATCH 1/3] fix: resolve all 5 bugs in book-library --- debugging/book-library/index.html | 4 +- debugging/book-library/script.js | 69 ++++++++++++++++++++----------- 2 files changed, 46 insertions(+), 27 deletions(-) diff --git a/debugging/book-library/index.html b/debugging/book-library/index.html index 23acfa71..6abc3b8b 100644 --- a/debugging/book-library/index.html +++ b/debugging/book-library/index.html @@ -1,4 +1,4 @@ - + @@ -65,7 +65,7 @@

Library

type="submit" value="Submit" class="btn btn-primary" - onclick="submit();" + onclick="submit()" /> diff --git a/debugging/book-library/script.js b/debugging/book-library/script.js index 75ce6c1d..d0e01ca4 100644 --- a/debugging/book-library/script.js +++ b/debugging/book-library/script.js @@ -1,13 +1,16 @@ +// myLibrary is the main array — like a shelf that holds all book objects let myLibrary = []; +// Wait for the page to fully load before running any code window.addEventListener("load", function (e) { - populateStorage(); - render(); + populateStorage(); // Add default books if the library is empty + render(); // Draw the table on screen }); +// Add two default books so the page is not empty on first load function populateStorage() { if (myLibrary.length == 0) { - let book1 = new Book("Robison Crusoe", "Daniel Defoe", "252", true); + let book1 = new Book("Robinson Crusoe", "Daniel Defoe", "252", true); let book2 = new Book( "The Old Man and the Sea", "Ernest Hemingway", @@ -16,33 +19,39 @@ function populateStorage() { ); myLibrary.push(book1); myLibrary.push(book2); - render(); } } +// Read the form input elements from the HTML const title = document.getElementById("title"); const author = document.getElementById("author"); const pages = document.getElementById("pages"); const check = document.getElementById("check"); -//check the right input from forms and if its ok -> add the new book (object in array) -//via Book function and start render function +// Called when the user clicks Submit — validates input then adds a new book function submit() { + // BUG 2 FIX: also check author field is not empty if ( title.value == null || title.value == "" || + author.value == null || + author.value == "" || pages.value == null || pages.value == "" ) { alert("Please fill all fields!"); return false; } else { - let book = new Book(title.value, title.value, pages.value, check.checked); - library.push(book); + // BUG 3 FIX: was title.value twice — now correctly uses author.value + let book = new Book(title.value, author.value, pages.value, check.checked); + // BUG 2 FIX: was "library" (undefined) — corrected to "myLibrary" + myLibrary.push(book); render(); } } +// Book is a constructor function — like a template for creating book objects +// Think of it as a blank form: every book fills in the same four fields function Book(title, author, pages, check) { this.title = title; this.author = author; @@ -50,54 +59,64 @@ function Book(title, author, pages, check) { this.check = check; } +// Redraws the entire table from scratch using the current myLibrary array function render() { let table = document.getElementById("display"); let rowsNumber = table.rows.length; - //delete old table - for (let n = rowsNumber - 1; n > 0; n-- { + + // Delete all existing rows except the header row (row 0) + // BUG 1 FIX: was "n-- {" — missing closing parenthesis, caused total JS failure + for (let n = rowsNumber - 1; n > 0; n--) { table.deleteRow(n); } - //insert updated row and cells + + // Loop through every book in myLibrary and build one table row per book let length = myLibrary.length; for (let i = 0; i < length; i++) { - let row = table.insertRow(1); - let titleCell = row.insertCell(0); - let authorCell = row.insertCell(1); - let pagesCell = row.insertCell(2); - let wasReadCell = row.insertCell(3); - let deleteCell = row.insertCell(4); + let row = table.insertRow(1); // Insert a new row after the header + let titleCell = row.insertCell(0); // First column: Title + let authorCell = row.insertCell(1); // Second column: Author + let pagesCell = row.insertCell(2); // Third column: Pages + let wasReadCell = row.insertCell(3); // Fourth column: Read button + let deleteCell = row.insertCell(4); // Fifth column: Delete button + + // Fill the text cells with data from the book object titleCell.innerHTML = myLibrary[i].title; authorCell.innerHTML = myLibrary[i].author; pagesCell.innerHTML = myLibrary[i].pages; - //add and wait for action for read/unread button + // Create the Read/Unread toggle button let changeBut = document.createElement("button"); changeBut.id = i; changeBut.className = "btn btn-success"; wasReadCell.appendChild(changeBut); + + // BUG 5 FIX: logic was inverted — true (read) now correctly shows "Yes" let readStatus = ""; - if (myLibrary[i].check == false) { + if (myLibrary[i].check == true) { readStatus = "Yes"; } else { readStatus = "No"; } changeBut.innerText = readStatus; + // Clicking the button flips the read status and redraws the table changeBut.addEventListener("click", function () { myLibrary[i].check = !myLibrary[i].check; render(); }); - //add delete button to every row and render again - let delButton = document.createElement("button"); + // BUG 4 FIX: variable was named "delButton" then used as "delBut" + // Now consistently named "delBut" throughout + // Also fixed: "clicks" event name changed to "click" + let delBut = document.createElement("button"); delBut.id = i + 5; deleteCell.appendChild(delBut); delBut.className = "btn btn-warning"; delBut.innerHTML = "Delete"; - delBut.addEventListener("clicks", function () { - alert(`You've deleted title: ${myLibrary[i].title}`); - myLibrary.splice(i, 1); - render(); + delBut.addEventListener("click", function () { + myLibrary.splice(i, 1); // Remove 1 book at index i from the array + render(); // Redraw the table without the deleted book }); } } From 23f883037b3c90ec4ee68fe59b86bfdfed84581d Mon Sep 17 00:00:00 2001 From: Mahmoud Shaabo Date: Fri, 17 Apr 2026 18:20:38 +0100 Subject: [PATCH 2/3] refactor: improve code quality based on CJ feedback --- debugging/book-library/index.html | 47 +++---- debugging/book-library/script.js | 206 ++++++++++++++++-------------- 2 files changed, 130 insertions(+), 123 deletions(-) diff --git a/debugging/book-library/index.html b/debugging/book-library/index.html index 6abc3b8b..2d5ff393 100644 --- a/debugging/book-library/index.html +++ b/debugging/book-library/index.html @@ -1,12 +1,10 @@ - + - - + + My Book Library + + @@ -29,22 +27,25 @@

Library

+ - + + + Library name="pages" required /> + + + +
@@ -80,15 +81,7 @@

Library

- - - - - - - - - + diff --git a/debugging/book-library/script.js b/debugging/book-library/script.js index d0e01ca4..92e1a43f 100644 --- a/debugging/book-library/script.js +++ b/debugging/book-library/script.js @@ -1,122 +1,136 @@ // myLibrary is the main array — like a shelf that holds all book objects let myLibrary = []; +// Use const for DOM references because these elements never change +// "const" means: this variable will always point to the same element +const titleInput = document.getElementById("title"); +const authorInput = document.getElementById("author"); +const pagesInput = document.getElementById("pages"); +const readCheckbox = document.getElementById("check"); + // Wait for the page to fully load before running any code -window.addEventListener("load", function (e) { - populateStorage(); // Add default books if the library is empty - render(); // Draw the table on screen +// addEventListener is safer than window.onload because it allows +// multiple listeners to be registered without overwriting each other +window.addEventListener("load", function () { + addDefaultBooks(); + render(); }); // Add two default books so the page is not empty on first load -function populateStorage() { - if (myLibrary.length == 0) { - let book1 = new Book("Robinson Crusoe", "Daniel Defoe", "252", true); - let book2 = new Book( - "The Old Man and the Sea", - "Ernest Hemingway", - "127", - true +// Renamed from "populateStorage" — that name implied localStorage usage +function addDefaultBooks() { + if (myLibrary.length === 0) { + myLibrary.push(new Book("Robinson Crusoe", "Daniel Defoe", "252", true)); + myLibrary.push( + new Book("The Old Man and the Sea", "Ernest Hemingway", "127", true) ); - myLibrary.push(book1); - myLibrary.push(book2); } } -// Read the form input elements from the HTML -const title = document.getElementById("title"); -const author = document.getElementById("author"); -const pages = document.getElementById("pages"); -const check = document.getElementById("check"); +// Book is a constructor function — a template for creating book objects +// Think of it as a blank contact card: every book fills in the same four fields +function Book(title, author, pages, hasBeenRead) { + this.title = title; + this.author = author; + this.pages = pages; + // Renamed "check" to "hasBeenRead" — much clearer what this field means + this.hasBeenRead = hasBeenRead; +} -// Called when the user clicks Submit — validates input then adds a new book -function submit() { - // BUG 2 FIX: also check author field is not empty +// Called when the user clicks Submit +// Renamed from "submit" to avoid conflict with the browser's built-in submit behaviour +function addBook() { + // Validate that all fields are filled before adding the book if ( - title.value == null || - title.value == "" || - author.value == null || - author.value == "" || - pages.value == null || - pages.value == "" + titleInput.value === "" || + authorInput.value === "" || + pagesInput.value === "" ) { alert("Please fill all fields!"); - return false; - } else { - // BUG 3 FIX: was title.value twice — now correctly uses author.value - let book = new Book(title.value, author.value, pages.value, check.checked); - // BUG 2 FIX: was "library" (undefined) — corrected to "myLibrary" - myLibrary.push(book); - render(); + return; } + + // Create a new Book object from the form values + const newBook = new Book( + titleInput.value, + authorInput.value, + pagesInput.value, + readCheckbox.checked + ); + + myLibrary.push(newBook); + + // Clear the form fields after adding the book + titleInput.value = ""; + authorInput.value = ""; + pagesInput.value = ""; + readCheckbox.checked = false; + + render(); } -// Book is a constructor function — like a template for creating book objects -// Think of it as a blank form: every book fills in the same four fields -function Book(title, author, pages, check) { - this.title = title; - this.author = author; - this.pages = pages; - this.check = check; +// createReadButton builds and returns a toggle button for a single book +// Extracting this into its own function makes render() easier to read +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; +} + +// createDeleteButton builds and returns a delete button for a single book +function createDeleteButton(index) { + const button = document.createElement("button"); + button.className = "btn btn-warning"; + button.textContent = "Delete"; + + button.addEventListener("click", function () { + // splice(index, 1) removes exactly 1 item at position "index" + myLibrary.splice(index, 1); + render(); + }); + + return button; } -// Redraws the entire table from scratch using the current myLibrary array +// render() redraws the entire table from the current myLibrary array function render() { - let table = document.getElementById("display"); - let rowsNumber = table.rows.length; + const table = document.getElementById("display"); + const rowCount = table.rows.length; - // Delete all existing rows except the header row (row 0) - // BUG 1 FIX: was "n-- {" — missing closing parenthesis, caused total JS failure - for (let n = rowsNumber - 1; n > 0; n--) { - table.deleteRow(n); + // Delete all rows except the header row (row 0) + for (let i = rowCount - 1; i > 0; i--) { + table.deleteRow(i); } - // Loop through every book in myLibrary and build one table row per book - let length = myLibrary.length; - for (let i = 0; i < length; i++) { - let row = table.insertRow(1); // Insert a new row after the header - let titleCell = row.insertCell(0); // First column: Title - let authorCell = row.insertCell(1); // Second column: Author - let pagesCell = row.insertCell(2); // Third column: Pages - let wasReadCell = row.insertCell(3); // Fourth column: Read button - let deleteCell = row.insertCell(4); // Fifth column: Delete button - - // Fill the text cells with data from the book object - titleCell.innerHTML = myLibrary[i].title; - authorCell.innerHTML = myLibrary[i].author; - pagesCell.innerHTML = myLibrary[i].pages; - - // Create the Read/Unread toggle button - let changeBut = document.createElement("button"); - changeBut.id = i; - changeBut.className = "btn btn-success"; - wasReadCell.appendChild(changeBut); - - // BUG 5 FIX: logic was inverted — true (read) now correctly shows "Yes" - let readStatus = ""; - if (myLibrary[i].check == true) { - readStatus = "Yes"; - } else { - readStatus = "No"; - } - changeBut.innerText = readStatus; - - // Clicking the button flips the read status and redraws the table - changeBut.addEventListener("click", function () { - myLibrary[i].check = !myLibrary[i].check; - render(); - }); - - // BUG 4 FIX: variable was named "delButton" then used as "delBut" - // Now consistently named "delBut" throughout - // Also fixed: "clicks" event name changed to "click" - let delBut = document.createElement("button"); - delBut.id = i + 5; - deleteCell.appendChild(delBut); - delBut.className = "btn btn-warning"; - delBut.innerHTML = "Delete"; - delBut.addEventListener("click", function () { - myLibrary.splice(i, 1); // Remove 1 book at index i from the array - render(); // Redraw the table without the deleted book - }); + // Build one row per book using helper functions + for (let i = 0; i < myLibrary.length; i++) { + const row = table.insertRow(); + const titleCell = row.insertCell(0); + const authorCell = row.insertCell(1); + const pagesCell = row.insertCell(2); + const readCell = row.insertCell(3); + const deleteCell = row.insertCell(4); + + // Use textContent for user data — never innerHTML — to prevent XSS attacks + // XSS (Cross-Site Scripting) is when a malicious script gets injected into a page + titleCell.textContent = myLibrary[i].title; + authorCell.textContent = myLibrary[i].author; + pagesCell.textContent = myLibrary[i].pages; + + // Append the buttons created by the helper functions + readCell.appendChild(createReadButton(i)); + deleteCell.appendChild(createDeleteButton(i)); } } From 111873993871946c2a92c831774c1c8d1b1eef99 Mon Sep 17 00:00:00 2001 From: Mahmoud Shaabo Date: Sat, 18 Apr 2026 11:23:07 +0100 Subject: [PATCH 3/3] refactor: apply CJ second round feedback --- debugging/book-library/script.js | 90 +++++++++++++++++--------------- 1 file changed, 48 insertions(+), 42 deletions(-) diff --git a/debugging/book-library/script.js b/debugging/book-library/script.js index 92e1a43f..881ee634 100644 --- a/debugging/book-library/script.js +++ b/debugging/book-library/script.js @@ -1,66 +1,83 @@ -// myLibrary is the main array — like a shelf that holds all book objects -let myLibrary = []; +// const is safer than let here because myLibrary is never reassigned — +// we only call .push() and .splice() on it, which modify its contents, not the variable itself +const myLibrary = []; -// Use const for DOM references because these elements never change -// "const" means: this variable will always point to the same element +// Declare all shared DOM references at the top of the file +// These are const because they always point to the same HTML elements const titleInput = document.getElementById("title"); const authorInput = document.getElementById("author"); const pagesInput = document.getElementById("pages"); const readCheckbox = document.getElementById("check"); // Wait for the page to fully load before running any code -// addEventListener is safer than window.onload because it allows -// multiple listeners to be registered without overwriting each other window.addEventListener("load", function () { addDefaultBooks(); render(); }); // Add two default books so the page is not empty on first load -// Renamed from "populateStorage" — that name implied localStorage usage function addDefaultBooks() { if (myLibrary.length === 0) { - myLibrary.push(new Book("Robinson Crusoe", "Daniel Defoe", "252", true)); + // Page count is stored as a NUMBER (252, not "252") — correct data type + myLibrary.push(new Book("Robinson Crusoe", "Daniel Defoe", 252, true)); myLibrary.push( - new Book("The Old Man and the Sea", "Ernest Hemingway", "127", true) + new Book("The Old Man and the Sea", "Ernest Hemingway", 127, true) ); } } // Book is a constructor function — a template for creating book objects -// Think of it as a blank contact card: every book fills in the same four fields function Book(title, author, pages, hasBeenRead) { this.title = title; this.author = author; this.pages = pages; - // Renamed "check" to "hasBeenRead" — much clearer what this field means this.hasBeenRead = hasBeenRead; } +// sanitiseText trims leading and trailing spaces from a string +// Prevents accepting inputs that contain only spaces as valid values +function sanitiseText(value) { + return value.trim(); +} + +// sanitisePages converts the pages input to a safe integer +// Rejects scientific notation like "3e2" which would display strangely +function sanitisePages(value) { + const trimmed = value.trim(); + // If the input contains "e" or "E", it is scientific notation — reject it + if (trimmed.toLowerCase().includes("e")) { + return NaN; + } + const parsed = Number(trimmed); + if (trimmed === "" || isNaN(parsed) || parsed <= 0) { + return NaN; + } + return Math.floor(parsed); +} + // Called when the user clicks Submit -// Renamed from "submit" to avoid conflict with the browser's built-in submit behaviour function addBook() { - // Validate that all fields are filled before adding the book - if ( - titleInput.value === "" || - authorInput.value === "" || - pagesInput.value === "" - ) { - alert("Please fill all fields!"); + // Step 1: Sanitise the inputs — clean them before checking or using them + const cleanTitle = sanitiseText(titleInput.value); + const cleanAuthor = sanitiseText(authorInput.value); + const cleanPages = sanitisePages(pagesInput.value); + + // Step 2: Validate — reject empty strings and invalid page numbers + if (cleanTitle === "" || cleanAuthor === "" || isNaN(cleanPages)) { + alert("Please fill all fields with valid values!"); return; } - // Create a new Book object from the form values + // Step 3: Create and store the new book using the cleaned values const newBook = new Book( - titleInput.value, - authorInput.value, - pagesInput.value, + cleanTitle, + cleanAuthor, + cleanPages, readCheckbox.checked ); - myLibrary.push(newBook); - // Clear the form fields after adding the book + // Step 4: Clear the form fields after adding the book titleInput.value = ""; authorInput.value = ""; pagesInput.value = ""; @@ -70,18 +87,12 @@ function addBook() { } // createReadButton builds and returns a toggle button for a single book -// Extracting this into its own function makes render() easier to read 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(); }); @@ -96,7 +107,8 @@ function createDeleteButton(index) { button.textContent = "Delete"; button.addEventListener("click", function () { - // splice(index, 1) removes exactly 1 item at position "index" + // Show an alert with the deleted book's title — restoring original behaviour + alert(`"${myLibrary[index].title}" has been deleted.`); myLibrary.splice(index, 1); render(); }); @@ -107,29 +119,23 @@ function createDeleteButton(index) { // render() redraws the entire table from the current myLibrary array function render() { 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); - } + // Clear the tbody in one step — simpler and faster than deleting rows one by one + const tbody = table.querySelector("tbody"); + tbody.innerHTML = ""; - // Build one row per book using helper functions for (let i = 0; i < myLibrary.length; i++) { - const row = table.insertRow(); + const row = tbody.insertRow(); const titleCell = row.insertCell(0); const authorCell = row.insertCell(1); const pagesCell = row.insertCell(2); const readCell = row.insertCell(3); const deleteCell = row.insertCell(4); - // Use textContent for user data — never innerHTML — to prevent XSS attacks - // XSS (Cross-Site Scripting) is when a malicious script gets injected into a page titleCell.textContent = myLibrary[i].title; authorCell.textContent = myLibrary[i].author; pagesCell.textContent = myLibrary[i].pages; - // Append the buttons created by the helper functions readCell.appendChild(createReadButton(i)); deleteCell.appendChild(createDeleteButton(i)); }