GLASGOW | May-2025 | Taras Mykytiuk | Sprint 2 | Book library #311

TarasMykytiuk · 2025-08-17T20:00:03Z

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

Briefly explain your PR.
In this branch was performed bugfixing of Book library app.

Questions

Ask any questions you have for your reviewer.

github-actions · 2025-08-17T20:00:25Z

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

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

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

github-actions · 2025-08-17T20:00:25Z

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

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

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

github-actions · 2025-08-17T20:00:30Z

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

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

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

github-actions · 2025-08-17T20:00:30Z

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

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

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

github-actions · 2025-08-17T20:00:33Z

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

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

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

github-actions · 2025-08-17T20:00:48Z

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

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

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

github-actions · 2025-08-17T20:02:56Z

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

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.

…e-Data-Flows into book_library update branch after review

TarasMykytiuk · 2025-08-18T20:53:35Z

Hi. Now I have taken feedback.md into account.

cjyuan · 2025-08-18T22:57:55Z

debugging/book-library/script.js

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


Why not just use the numeric literal 252?

Just did not think about it)

cjyuan · 2025-08-18T23:03:55Z

debugging/book-library/script.js

+  const div = document.createElement('div');
+  div.textContent = str;
+  return div.innerHTML;


This turns special characters to HTML entities. For example, > to >.
Have you checked what would happen if you include a character & in the title?

You can ask AI why this conversion is not needed when the input is assigned to the .textContent or .innerText properties.

Thanks. Now I will consider this in future.

cjyuan · 2025-08-18T23:05:23Z

debugging/book-library/script.js

+function isHexadecimal(input) {
+  return /^0x[0-9a-fA-F]+$/.test(input);
+}


Why would you want to allow the user to enter a page count as a hexadecimal number?

I dont allow. When user try to input something like: "0x10" - this function return true and condition in line 50 trigger error alert window in line 53.

cjyuan · 2025-08-18T23:06:26Z

debugging/book-library/script.js

+    bookTitleInput.value.trim() == "" ||
+    bookAuthorInput.value.trim() == "" ||
+    bookNumberOfPagesInput.value.trim() == ""
  ) {
    alert("Please fill all fields!");
    return false;
-  } else {
-    let book = new Book(title.value, author.value, pages.value, check.checked);
+  } else if (
+    isNaN(Number(bookNumberOfPagesInput.value.trim())) ||
+    !isValidInteger(Number(bookNumberOfPagesInput.value.trim())) ||
+    isHexadecimal(bookNumberOfPagesInput.value.trim()) ||
+    Number(bookNumberOfPagesInput.value.trim()) <= 0
+  ) {
+    alert("Invalid number of pages format!");
+    return false;
+  }  else {
+    const bookTitle = sanitizeInput(bookTitleInput.value.trim());
+    const bookAuthor = sanitizeInput(bookAuthorInput.value.trim());
+    const bookNumberOfPages = parseInt(sanitizeInput(bookNumberOfPagesInput.value.trim()), 10);
+    let book = new Book(bookTitle, bookAuthor, bookNumberOfPages, isBookReadCheckBox.checked);


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.

Number.isInteger() would also return false when the input is not a number; we can omit isNaN() check.

Yes. Now is more efficient.

cjyuan · 2025-08-18T23:11:22Z

debugging/book-library/script.js

    let readStatus = "";
    if (myLibrary[i].check == false) {
      readStatus = "No";
    } else {
      readStatus = "Yes";
    }
-    changeBut.innerText = readStatus;
+    changeReadStatusButton.innerText = readStatus;


This could be a good opportunity to practice using the ? : conditional operator. Can you rewrite the code on lines 99-105 as a single statement?

Thank. I just missed it.

cjyuan

Changes look good.

cjyuan · 2025-08-19T12:25:42Z

debugging/book-library/index.html

        <input
-          type="number"
+          type="text"
          class="form-control"
          id="pages"
          name="pages"
          required
-        />


Why change type="number" to type="text" when Page count is supposed to be an integer? If anything, we should make the check stricter (to allow only digits in the input field).

)) Yes. Now fixed.

cjyuan · 2025-08-19T12:29:14Z

debugging/book-library/script.js

-      readStatus = "No";
-    }
-    changeBut.innerText = readStatus;
+    !myLibrary[i].check ? readStatus = "No" : readStatus = "Yes";


The common way to use ? : is
readStatus = !myLibrary[i].check ? "No" : "Yes";

!myLibrary[i].check ? "No" : "Yes" is an expression that evaluates to either "No" or "Yes".

Thanks for review!

TarasMykytiuk and others added 3 commits August 17, 2025 20:42

first commit

896cf58

bugfixing

7b319e7

Merge branch 'CodeYourFuture:main' into book_library

1b78ee4

TarasMykytiuk added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 17, 2025

TarasMykytiuk mentioned this pull request Aug 17, 2025

GLASGOW | May-2025 | Data flows | Taras Mykytiuk TarasMykytiuk/My-Coursework-Planner#54

Open

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

TarasMykytiuk added 3 commits August 18, 2025 20:45

index.html fixed

ff05f0f

Merge branch 'book_library' of https://github.com/TarasMykytiuk/Modul…

af12243

…e-Data-Flows into book_library update branch after review

script.js fixed according to feedback.md

6dcafb4

TarasMykytiuk added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 18, 2025

TarasMykytiuk removed the Reviewed Volunteer to add when completing a review with trainee action still to take. label Aug 18, 2025

cjyuan reviewed Aug 18, 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 18, 2025

reaction to feedback

bafe966

TarasMykytiuk 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

reaction to feedback

5ed9f1c

illicitonion closed this Sep 1, 2025

Uh oh!

GLASGOW | May-2025 | Taras Mykytiuk | Sprint 2 | Book library #311

GLASGOW | May-2025 | Taras Mykytiuk | Sprint 2 | Book library #311

Uh oh!

Conversation

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

Uh oh!

Learners, PR Template

Changelist

Questions

Uh oh!

github-actions bot commented Aug 17, 2025

Uh oh!

github-actions bot commented Aug 17, 2025

Uh oh!

github-actions bot commented Aug 17, 2025

Uh oh!

github-actions bot commented Aug 17, 2025

Uh oh!

github-actions bot commented Aug 17, 2025

Uh oh!

github-actions bot commented Aug 17, 2025

Uh oh!

github-actions bot commented Aug 17, 2025

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

TarasMykytiuk commented Aug 18, 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!

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!

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!

3 participants

TarasMykytiuk commented Aug 17, 2025 •

edited

Loading