Skip to content

London | 26-ITP-Jan | Boualem Larbi Djebbour | Sprint 2 | Book Library #482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
djebsoft wants to merge 1 commit into from
Closed

London | 26-ITP-Jan | Boualem Larbi Djebbour | Sprint 2 | Book Library #482

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 10 additions & 14 deletions debugging/book-library/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ window.addEventListener("load", function (e) {

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",
Expand All @@ -16,7 +16,6 @@ function populateStorage() {
);
myLibrary.push(book1);
myLibrary.push(book2);
render();
}
}

Expand All @@ -31,14 +30,16 @@ function submit() {
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);
let book = new Book(title.value, author.value, pages.value, check.checked);
myLibrary.push(book);
render();
}
}
Comment on lines 30 to 45
Copy link
Copy Markdown

@Luro91 Luro91 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before using input in an app, we should properly preprocess it (even if basic validation has already been enforced in HTML):

  • Input Validation - Check whether the input meets expected formats, types, and constraints
  • Input Sanitization - Clean input to exclude unwanted or potentially harmful data
  • Input Normalization/Conversion - Transform input into the correct data type or a standardized, usable format

Questions to consider:

  • Should title and author be allowed to contain only space characters?
  • Should title and author be allowed to contain leading or trailing space characters?
  • What type of value should we use to store the page count?
  • What kinds of input values (if any) should be rejected?

Expand All @@ -54,7 +55,7 @@ function render() {
let table = document.getElementById("display");
Copy link
Copy Markdown

@Luro91 Luro91 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When setting the text content of an HTML element, there are subtle but important differences between using .innerHTML, innerText, and textContent.

In render(), are the text values assigned to the appropriate property of each DOM node?

let rowsNumber = table.rows.length;
//delete old table
for (let n = rowsNumber - 1; n > 0; n-- {
for (let n = rowsNumber - 1; n > 0; n--) {
table.deleteRow(n);
}
Comment on lines +58 to 60
Copy link
Copy Markdown

@Luro91 Luro91 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Efficient approach to remove rows in <table>

In render(), instead of deleting table rows one by one, we could delete all rows in one operation by clearing their container.

//insert updated row and cells
Expand All @@ -75,26 +76,21 @@ function render() {
changeBut.id = i;
changeBut.className = "btn btn-success";
wasReadCell.appendChild(changeBut);
let readStatus = "";
if (myLibrary[i].check == false) {
readStatus = "Yes";
} else {
readStatus = "No";
}
changeBut.innerText = readStatus;

changeBut.innerText = myLibrary[i].check ? "Yes" : "No";
Copy link
Copy Markdown

@Luro91 Luro91 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of the ternary operator


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");
let delBut = document.createElement("button");
Copy link
Copy Markdown

@Luro91 Luro91 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Are the values assigned to the id attributes of the delete button unique?
  • Is there any need to assign an id attribute to the buttons?

delBut.id = i + 5;
deleteCell.appendChild(delBut);
delBut.className = "btn btn-warning";
delBut.innerHTML = "Delete";
delBut.addEventListener("clicks", function () {
delBut.addEventListener("click", function () {
alert(`You've deleted title: ${myLibrary[i].title}`);
myLibrary.splice(i, 1);
Comment on lines 94 to 95
Copy link
Copy Markdown

@Luro91 Luro91 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the callback function of the delete button, 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.

render();
Expand Down
Loading