London | 26-ITP-Jan | Abdul Moiz | Sprint 2 | Book Library by A-Moiz · Pull Request #385 · CodeYourFuture/Module-Data-Flows

A-Moiz · 2026-03-03T14:26:26Z

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

Resolved all known bugs
Reset all values after adding book to improve UX

Questions

N/A

ykamal

Nice work! 👍 Things look mostly right. Well done. There are a few things that need a bit of work. I've left comments below. Let me know if you have any questions.

ykamal · 2026-03-16T22:22:04Z

debugging/book-library/script.js

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


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.

ykamal · 2026-03-16T22:23:35Z

debugging/book-library/script.js

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


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

ykamal · 2026-03-16T22:27:46Z

debugging/book-library/script.js

-    pages.value == null ||
-    pages.value == ""
-  ) {
+  if (title.value === "" || author.value === "" || pages.value === "") {


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?

ykamal · 2026-03-16T22:29:31Z

debugging/book-library/script.js

+    let book = new Book(
+      title.value,
+      author.value,
+      String(pages.value),


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

ykamal · 2026-03-16T22:30:30Z

debugging/book-library/script.js

+    this.title = title;
+    this.author = author;
+    this.pages = pages;
+    this.check = check;


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

and data range (numbers)

ykamal · 2026-03-16T22:33:49Z

debugging/book-library/script.js

render() appears to be called twice on page load. Perhaps you should create a sequence of this from the root function call and not inside here?

ykamal · 2026-03-16T22:40:33Z

debugging/book-library/script.js

+    } else {
+      readStatus = "Yes";
    }
    changeBut.innerText = readStatus;


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

ykamal · 2026-03-16T22:45:13Z

debugging/book-library/script.js

-    delBut.className = "btn btn-warning";
-    delBut.innerHTML = "Delete";
-    delBut.addEventListener("clicks", function () {
+    delButton.id = i + 5;


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.

ykamal · 2026-03-16T22:47:57Z

debugging/book-library/script.js

+    delButton.id = i + 5;
+    deleteCell.appendChild(delButton);
+    delButton.className = "btn btn-warning";
+    delButton.innerHTML = "Delete";


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.

ykamal · 2026-03-16T22:49:36Z

debugging/book-library/script.js

+    deleteCell.appendChild(delButton);
+    delButton.className = "btn btn-warning";
+    delButton.innerHTML = "Delete";
+    delButton.addEventListener("click", function (e) {


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

A-Moiz · 2026-03-18T00:19:22Z

Hi @ykamal , I've made all the changes you requested. Let me know if you anymore recommendations.

Resolved all known bugs

9e9749a

A-Moiz added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 3, 2026

ykamal added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 16, 2026

ykamal suggested changes Mar 16, 2026

View reviewed changes

ykamal added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Mar 16, 2026

Made changes based on reviewers comments

36bcbcb

A-Moiz added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Mar 18, 2026

A-Moiz mentioned this pull request Mar 18, 2026

London | Data Flows | Abdul Moiz A-Moiz/My-Coursework-Planner#97

Open

Uh oh!

Conversation

A-Moiz commented Mar 3, 2026

Learners, PR Template

Changelist

Questions

Uh oh!

ykamal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A-Moiz commented Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants