Skip to content

London | 26-ITP-Jan | Abdul Moiz | Sprint 2 | Book Library #385

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
A-Moiz wants to merge 2 commits into
base: main
Choose a base branch
from
Open

London | 26-ITP-Jan | Abdul Moiz | Sprint 2 | Book Library #385

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e9749a
Resolved all known bugs
A-Moiz Mar 3, 2026
36bcbcb
Made changes based on reviewers comments
A-Moiz Mar 18, 2026
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
52 changes: 29 additions & 23 deletions debugging/book-library/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ window.addEventListener("load", function (e) {
});

function populateStorage() {
if (myLibrary.length == 0) {
if (myLibrary.length === 0) {
let book1 = new Book("Robison Crusoe", "Daniel Defoe", "252", true);
Copy link

@ykamal ykamal Mar 16, 2026

Choose a reason for hiding this comment

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

Love to see some OOP code. Since the class you're referencing here is declared further down the file, for the sake of readability and code organisation, I recommend moving the class definition to much higher, or to the beginning of this file.

Copy link

@ykamal ykamal Mar 16, 2026

Choose a reason for hiding this comment

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

book1 is not getting reassigned later, so using a const keyword will make it safer in the same scope. It's still going to allow you to run object mutations where applicable. but it ensures that you cannot redeclare a variable with the same name inside the same scope.
Same for book2

let book2 = new Book(
"The Old Man and the Sea",
Expand All @@ -28,33 +28,39 @@ 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() {
if (
title.value == null ||
title.value == "" ||
pages.value == null ||
pages.value == ""
) {
if (title.value === "" || author.value === "" || pages.value === "") {
Copy link

@ykamal ykamal Mar 16, 2026

Choose a reason for hiding this comment

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

Good validation check 👍
However, if you do not remove unnecessary whitespaces from the start and the end, " " will pass through this check as it's a bunch of valid characters and is not an empty string. How do you think you could counter this?

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,
String(pages.value),
Copy link

@ykamal ykamal Mar 16, 2026

Choose a reason for hiding this comment

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

This is good safe practice, but there is most likely no need to convert this into a string, as all input values are always returned (usually) as strings, unless something like this is used: https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/valueAsNumber

check.checked
);
myLibrary.push(book);
render();
title.value = "";
author.value = "";
pages.value = "";
check.checked = false;
}
}

function Book(title, author, pages, check) {
this.title = title;
this.author = author;
this.pages = pages;
this.check = check;
class Book {
constructor(title, author, pages, check) {
this.title = title;
this.author = author;
this.pages = pages;
this.check = check;
Copy link

@ykamal ykamal Mar 16, 2026

Choose a reason for hiding this comment

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

Since this is JavaScript, do you think we should ensure that these constructor arguments are provided in the right data type?

Copy link

@ykamal ykamal Mar 16, 2026

Choose a reason for hiding this comment

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

  • and data range (numbers)

}
}

function render() {
let table = document.getElementById("display");
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);
}
//insert updated row and cells
Expand All @@ -76,10 +82,10 @@ function render() {
changeBut.className = "btn btn-success";
wasReadCell.appendChild(changeBut);
let readStatus = "";
if (myLibrary[i].check == false) {
readStatus = "Yes";
} else {
if (myLibrary[i].check === false) {
readStatus = "No";
} else {
readStatus = "Yes";
}
changeBut.innerText = readStatus;
Copy link

@ykamal ykamal Mar 16, 2026

Choose a reason for hiding this comment

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

Have you come across Ternary Operator? Could make the code cleaner:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Conditional_operator


Expand All @@ -90,11 +96,11 @@ function render() {

//add delete button to every row and render again
let delButton = document.createElement("button");
delBut.id = i + 5;
deleteCell.appendChild(delBut);
delBut.className = "btn btn-warning";
delBut.innerHTML = "Delete";
delBut.addEventListener("clicks", function () {
delButton.id = i + 5;
Copy link

@ykamal ykamal Mar 16, 2026

Choose a reason for hiding this comment

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

I can see that you are trying to avoid duplicate IDs. Nice approach. However, the approach is hard-coding a specific number, and the outcome could be inconsistent.
You could just use delButton.id = "delButton_" + i or such tactics. That'll make them truly unique. Obviously, use string interpolation with backticks. I can't write that here.

deleteCell.appendChild(delButton);
delButton.className = "btn btn-warning";
delButton.innerHTML = "Delete";
Copy link

@ykamal ykamal Mar 16, 2026

Choose a reason for hiding this comment

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

use textContent as it parses the provided value as a string, whereas innerHTML parses it as HTML and should only be used if the change contains actual HTML. You can also use innerText for static text. textContent may be faster for most things.

delButton.addEventListener("click", function (e) {
Copy link

@ykamal ykamal Mar 16, 2026

Choose a reason for hiding this comment

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

a better way to do this would be using data- attributes. You're using i inside a closure which, while fine, is not the best practice.
jQuery has excellent APIs for using data attributes. Even with core JavaScript, if you can master data attributes, you will no longer need to do these, and can use HTML itself for storing that reference.
https://developer.mozilla.org/en-US/docs/Web/HTML/How_to/Use_data_attributes

alert(`You've deleted title: ${myLibrary[i].title}`);
myLibrary.splice(i, 1);
render();
Expand Down
Loading