Sheffield | May-2025 | Waleed-Yahay Salih-Taha | Sprint 2 | Book Library by Bluejay600 · Pull Request #286 · CodeYourFuture/Module-Data-Flows

Bluejay600 · 2025-08-09T03:01:02Z

Learners, PR Template

Self checklist

I have committed my files one by one, on purpose, and for a reason
I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
I have tested my changes
My changes follow the style guide
My changes meet the requirements of this task

Changelist

this PR is to View a list of books that I've read Add books to a list of books that I've read, Including title, author, number of pages and if I've read.

Questions

Ask any questions you have for your reviewer.

…ner.

…ata-Flows into book-library

cjyuan

Can you check if any of this general feedback can help you further improve your code?
https://github.com/cjyuan/Module-Data-Flows/blob/book-library-feedback/debugging/book-library/feedback.md

Doing so can help me speed up the review process. Thanks.

In addition, can you change the base branch of this PR from CYF's book-library to CYF's main?

github-actions · 2025-08-16T11:39:11Z

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Wrong number of parts separated by |s

github-actions · 2025-08-16T11:43:59Z

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Sprint-2) doesn't match expected format (example: 'Sprint 2', without quotes)

github-actions · 2025-08-16T11:53:44Z

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Flow Sprint 2) doesn't match expected format (example: 'Sprint 2', without quotes)

Bluejay600 · 2025-08-17T01:38:14Z

@cjyuan you can now review it, it's ready.

cjyuan · 2025-08-17T10:33:34Z

debugging/book-library/script.js

 const pages = document.getElementById("pages");
 const check = document.getElementById("check");

+document.getElementById("submitBtn").addEventListener("click", addBook);


It is better to group all the code that's to be executed once on page load together in the window.onload callback function (around line 4)

now I Grouped all setup code in window.onload

cjyuan · 2025-08-17T10:39:28Z

debugging/book-library/script.js

+  if (!title.value.trim() || !author.value.trim() || !pages.value.trim()) {
+    alert("Please fill all fields with valide values (no empty spaces)!");
+    return;
+  }
+    let book = new Book(title.value.trim(), author.value.trim(), pages.value.trim(), check.checked);
+    myLibrary.push(book);


For better performance (reduce number of function calls) and reducing the chance of using raw input accidently, we could stored the pre-processed/sanitized/normalized input in some variables first, and reference the variables in other part of the function.

pages.value.trim() is a string, without proper validation, sanitization, and pre-processing, the following page numbers are possible:

Pre-processed and sanitizes the input before using it multiple times

cjyuan · 2025-08-17T10:40:38Z

debugging/book-library/script.js

+      alert(`You've deleted title: ${book.title}`);
      myLibrary.splice(i, 1);
      render();


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.

Fixed the delete alert order (show confirmation only after deletion succeeds, not before)

cjyuan · 2025-08-17T10:46:18Z

debugging/book-library/script.js

 const pages = document.getElementById("pages");
 const check = document.getElementById("check");


Using descriptive and consistent suffixes (like El, Input, Btn, Form, etc.) for variables that store DOM elements can improve code readability and maintainability.

I have Consistented suffixes for DOM elements (El, Btn, Form, etc.)

…s, not before)

… prevent extremely large numbers of pages (e.g., 9999999). checks if author/title length is within reasonable range.

cjyuan

Changes look good.

cjyuan · 2025-08-19T01:58:52Z

debugging/book-library/script.js

+  const titleInputEl = document.getElementById("title");
+  const authorInputEl = document.getElementById("author");
+  const pagesInputEl = document.getElementById("pages");
+  const readCheckEl = document.getElementById("check");
+  const submitBtnEl = document.getElementById("submitBtn");
+  const bookFormEl = document.getElementById("bookForm");
+  const tbodyEl = document.querySelector("#display tbody");


These set of code could be made an exception and place outside window.onload.
Otherwise, the declared variables can only be accessed locally within the callback function.

cjyuan · 2025-08-19T02:02:17Z

debugging/book-library/script.js

+  alert("Author must be at least 2 characters long.");
+  return;
+}
+const pageNum = Number(pages);


If you place this line of code at the beginning of this function, you could then reference the value of this variable at lines 37.

cjyuan · 2025-08-19T02:03:50Z

debugging/book-library/script.js

+if (!Number.isInteger(pageNum) || pageNum <= 0 || pageNum > 10000) {
+  alert("Pages must be a positive whole number (1–10,000).");
+  return;
 }

-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);
+    // Add book
+    let book = new Book(title, author, pages, read);
+    myLibrary.push(book);
+    render();
+    bookFormEl.reset();
  }


Why not use the VSCode extension prettier to auto format/indent the code?

aidazhukova and others added 4 commits March 28, 2025 16:15

bugs are fixed now

c830178

fixed the if condition's syntax of the missing bracket.

c3b7f29

added a title to the web page and a languagge element to the html ope…

e99d6aa

…ner.

Merge branch 'book-library' of https://github.com/Bluejay600/Module-D…

ff68704

…ata-Flows into book-library

Bluejay600 added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Data-Flows The name of the module. labels Aug 9, 2025

cjyuan reviewed Aug 9, 2025

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 9, 2025

Bluejay600 changed the base branch from book-library to main August 16, 2025 11:38

Bluejay600 added 2 commits August 16, 2025 14:04

updated html code

3869805

updated css.

300dc39

Bluejay600 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 Aug 16, 2025

Bluejay600 added 4 commits August 16, 2025 15:45

updated the script file.

6b2c9a9

updated and validated html file.

c21b787

updated the input values not to be empty spaces.

5fd2034

updated css file.

808cd28

cjyuan reviewed Aug 17, 2025

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 17, 2025

Cache DOM elements with clear suffixes

6473c1f

Bluejay600 added 7 commits August 19, 2025 00:34

Grouped all setup codes in window.onload

60240cb

improved function to addbook

196a070

Fix delete alert order (show confirmation only after deletion succeed…

2e0943e

…s, not before)

updated html code.

555c393

fixes js code

410104f

Improve maintainability / readability

b77a783

Edited input validation to prevent decimal values (e.g., 12.5 pages).…

f032d63

… prevent extremely large numbers of pages (e.g., 9999999). checks if author/title length is within reasonable range.

Bluejay600 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 Aug 19, 2025

cjyuan reviewed Aug 19, 2025

View reviewed changes

cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 19, 2025

illicitonion closed this Sep 1, 2025

		const pages = document.getElementById("pages");
		const check = document.getElementById("check");

Uh oh!

Conversation

Bluejay600 commented Aug 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Learners, PR Template

Changelist

Questions

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 16, 2025

Uh oh!

github-actions bot commented Aug 16, 2025

Uh oh!

github-actions bot commented Aug 16, 2025

Uh oh!

Bluejay600 commented Aug 17, 2025

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!

cjyuan 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Bluejay600 commented Aug 9, 2025 •

edited

Loading