Skip to content

London | 25-ITP-Sep | Adnaan Abo | Sprint 2 | book library project #344

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

Open
AdnaanA wants to merge 5 commits into
base: main
Choose a base branch
from
Open

London | 25-ITP-Sep | Adnaan Abo | Sprint 2 | book library project #344

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
450911b
Refactor HTML structure, improve form handling, and update README for…
AdnaanA Dec 7, 2025
ac6bc70
Update button functionality to add books and improve event handling
AdnaanA Dec 9, 2025
ea76889
Optimize render function to clear table rows more efficiently
AdnaanA Dec 9, 2025
eea5314
Refactor addBook function for improved validation and sanitization
AdnaanA Dec 10, 2025
aadf229
Refactor render function for improved efficiency and clarity in cell …
AdnaanA Dec 10, 2025
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
66 changes: 34 additions & 32 deletions debugging/book-library/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,22 @@
<!DOCTYPE html>
<html>
<html lang="en">
<head>
<title> </title>
<meta charset="utf-8" />
<meta
charset="utf-8"
name="viewport"
content="width=device-width, initial-scale=1.0"
/>
<title>My Library</title>

<!-- jQuery, Popper, Bootstrap -->
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.4.1/jquery.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.16.0/umd/popper.min.js"></script>
<script src="https://maxcdn.bootstrapcdn.com/bootstrap/4.4.1/js/bootstrap.min.js"></script>
<link
rel="stylesheet"
href="https://maxcdn.bootstrapcdn.com/bootstrap/4.4.1/css/bootstrap.min.css"
/>

<link rel="stylesheet" type="text/css" href="style.css" />
</head>

Expand All @@ -23,71 +26,70 @@ <h1>Library</h1>
<p>Add books to your virtual library</p>
</div>

<button data-toggle="collapse" data-target="#demo" class="btn btn-info">
<button data-toggle="collapse" data-target="#demo" class="btn btn-info mb-3">
Add new book
</button>

<div id="demo" class="collapse">
<div class="form-group">
<label for="title">Title:</label>
<form id="bookForm" class="form-group">
<label for="book-title">Book title:</label>
<input
type="title"
type="text"
class="form-control"
id="title"
name="title"
id="book-title"
name="book-title"
required
/>
<label for="author">Author: </label>

<label for="author" class="mt-2">Author:</label>
<input
type="author"
type="text"
class="form-control"
id="author"
name="author"
required
/>
<label for="pages">Pages:</label>

<label for="pages" class="mt-2">Pages:</label>
<input
type="number"
class="form-control"
id="pages"
name="pages"
required
/>
<label class="form-check-label">

<div class="form-check mt-2">
<input
type="checkbox"
class="form-check-input"
id="check"
value=""
/>Read
</label>
<input
type="submit"
value="Submit"
class="btn btn-primary"
onclick="submit();"
/>
</div>
/>
<label class="form-check-label" for="check">Read</label>
</div>

<button
type="button"
class="btn btn-primary mt-3"
onclick="addBook()"
Copy link
Contributor

@cjyuan cjyuan Dec 9, 2025

Choose a reason for hiding this comment

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

It's a better practice to "attach event listener via JS" than "mixing JS code in HTML".

Copy link
Author

@AdnaanA AdnaanA Dec 9, 2025

Choose a reason for hiding this comment

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

added event listener via JS.

>
Submit
</button>
</form>
</div>

<table class="table" id="display">
<table class="table mt-4" id="display">
<thead class="thead-dark">
<tr>
<th>Title</th>
<th>Author</th>
<th>Number of Pages</th>
<th>Pages</th>
<th>Read</th>
<th></th>
</tr>
</thead>
<tbody>
<tr>
<td></td>
<td></td>
<td></td>
<td></td>
<td></td>
</tr>
<!-- rows inserted by JS -->
</tbody>
</table>

Expand Down
12 changes: 6 additions & 6 deletions debugging/book-library/readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ My website should be able to:

## Bugs to be fixed

1. Website loads but doesn't show any books
2. Error in console when you try to add a book
3. It uses the title name as the author name
4. Delete button is broken
5. When I add a book that I say I've read - it saves the wrong answer
1. Website loads but doesn't show any books. Done
2. Error in console when you try to add a book. Done
3. It uses the title name as the author name. Done
4. Delete button is broken. Done
5. When I add a book that I say I've read - it saves the wrong answer. Done

I think there are other some other small bugs in my code...but I'm lazy so I can't fix them all.
I think there are some other small bugs in my code...but I'm lazy so I can't fix them all.

I wish somebody would help me!
114 changes: 50 additions & 64 deletions debugging/book-library/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,103 +1,89 @@
let myLibrary = [];

window.addEventListener("load", function (e) {
window.addEventListener("load", function () {
populateStorage();
render();
});

function populateStorage() {
if (myLibrary.length == 0) {
let book1 = new Book("Robison Crusoe", "Daniel Defoe", "252", true);
let book2 = new Book(
"The Old Man and the Sea",
"Ernest Hemingway",
"127",
true
);
myLibrary.push(book1);
myLibrary.push(book2);
render();
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));
}
}

const title = document.getElementById("title");
const bookTitle = document.getElementById("book-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
function submit() {
// renamed so we don’t conflict with form.submit()
function addBook() {
if (
title.value == null ||
title.value == "" ||
pages.value == null ||
pages.value == ""
!bookTitle.value.trim() ||
!author.value.trim() ||
!pages.value.trim()
) {
alert("Please fill all fields!");
return false;
} else {
let book = new Book(title.value, title.value, pages.value, check.checked);
library.push(book);
render();
return;
}

const book = new Book(
bookTitle.value.trim(),
author.value.trim(),
pages.value.trim(),
Copy link
Contributor

@cjyuan cjyuan Dec 9, 2025

Choose a reason for hiding this comment

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

  • It is a good practice to save sanitized or pre-processed values in some variables first

    • to avoid calling .trim() repeatedly, and
    • to reduce the chance of accidently using the raw value.

  • The book's page property can still be assigned some invalid page count.

Copy link
Author

@AdnaanA AdnaanA Dec 10, 2025

Choose a reason for hiding this comment

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

we can clean the values once and store them in variables: instead of the above code we could do
const titleValue = bookTitle.value.trim();
const authorValue = author.value.trim();
const pagesValue = pages.value.trim();

and we can over come the page being assigned invalid count we can do this

const pagesNumber = Number(pagesValue);
if (!Number.isInteger(pagesNumber) || pagesNumber <= 0) {
alert("Please enter a valid positive page count!");
return;
}

check.checked
);

myLibrary.push(book);
render();
}

function Book(title, author, pages, check) {
this.title = title;
function Book(bookTitle, author, pages, check) {
this.bookTitle = bookTitle;
this.author = author;
this.pages = pages;
this.check = check;
}

function render() {
let table = document.getElementById("display");
let rowsNumber = table.rows.length;
//delete old table
for (let n = rowsNumber - 1; n > 0; n-- {
table.deleteRow(n);
const table = document.getElementById("display");

// remove all rows except the header
while (table.rows.length > 1) {
table.deleteRow(1);
}
Copy link
Contributor

@cjyuan cjyuan Dec 9, 2025

Choose a reason for hiding this comment

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

To clear the table before repopulating it, instead of deleting the table rows one by one, can you think of a more efficient way to remove all rows (except the <th>...</th>) in the table?

Copy link
Author

@AdnaanA AdnaanA Dec 9, 2025

Choose a reason for hiding this comment

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

we can use tbody.innerHTML = ''; to clear all rows efficiently.

however we have to use const row = tbody.insertRow(); to insert rows into tbody.

//insert updated row and cells
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);
titleCell.innerHTML = myLibrary[i].title;
authorCell.innerHTML = myLibrary[i].author;
pagesCell.innerHTML = myLibrary[i].pages;

//add and wait for action for read/unread button
let changeBut = document.createElement("button");
changeBut.id = i;
myLibrary.forEach((book, i) => {
const row = table.insertRow(1);

row.insertCell(0).textContent = book.bookTitle;
row.insertCell(1).textContent = book.author;
row.insertCell(2).textContent = book.pages;

// read/unread toggle
const wasReadCell = row.insertCell(3);
const changeBut = document.createElement("button");
changeBut.className = "btn btn-success";
changeBut.textContent = book.check ? "Yes" : "No";
wasReadCell.appendChild(changeBut);
let readStatus = "";
if (myLibrary[i].check == false) {
readStatus = "Yes";
} else {
readStatus = "No";
}
changeBut.innerText = readStatus;

changeBut.addEventListener("click", function () {
myLibrary[i].check = !myLibrary[i].check;
changeBut.addEventListener("click", () => {
book.check = !book.check;
render();
});

//add delete button to every row and render again
let delButton = document.createElement("button");
delBut.id = i + 5;
deleteCell.appendChild(delBut);
// delete button
const deleteCell = row.insertCell(4);
Copy link
Contributor

@cjyuan cjyuan Dec 9, 2025

Choose a reason for hiding this comment

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

Can you think of the pros and cons of these two approaches for creating cells within a row?

  • Keeping all the cell creation code in one location, like the original code does.
  • Scattering the cell creation code across different locations, like what you did.

Copy link
Author

@AdnaanA AdnaanA Dec 10, 2025

Choose a reason for hiding this comment

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

Keeping all cells created in one block is the best practice.
It is clearer, safer, easier to maintain, and less likely to produce bugs.
while
Scattered cell creation should only be used when absolutely necessary for conditional or dynamic table structures.

const delBut = document.createElement("button");
delBut.className = "btn btn-warning";
delBut.innerHTML = "Delete";
delBut.addEventListener("clicks", function () {
alert(`You've deleted title: ${myLibrary[i].title}`);
delBut.textContent = "Delete";
deleteCell.appendChild(delBut);

delBut.addEventListener("click", () => {
alert(`You've deleted: ${book.bookTitle}`);
myLibrary.splice(i, 1);
render();
});
}
});
}
6 changes: 6 additions & 0 deletions debugging/book-library/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,9 @@
button.btn-info {
margin: 20px;
}

@media (min-width: 576px) {
.jumbotron {
padding: 2rem 32px 2rem;
}
}
Loading