Skip to content

Conversation

MansoorM11
Copy link

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

Briefly explain your PR.
fixed the bugs in book library and refactor code to meet the requirements of the task. And also used "https://github.com/cjyuan/Module-Data-Flows/blob/book-library-feedback/debugging/book-library/feedback.md" to improve my code

Questions

Ask any questions you have for your reviewer.

@MansoorM11 MansoorM11 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 20, 2025
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Most of the the suggestions in the feedback document I shared with you were not taken into consideration. Please go through the document thoroughly.

@@ -7,7 +7,7 @@ window.addEventListener("load", function (e) {

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

Choose a reason for hiding this comment

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

Why change one and not the other?

Copy link
Author

Choose a reason for hiding this comment

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

i totally missed that. Thanks for letting me know

Comment on lines 77 to 83
let readStatus = "";
if (myLibrary[i].check == false) {
readStatus = "Yes";
} else {
readStatus = "No";
} else {
readStatus = "Yes";
}
changeBut.innerText = readStatus;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@cjyuan 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 20, 2025
@MansoorM11 MansoorM11 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 20, 2025
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

According to https://validator.w3.org/, there are errors in your index.html. Can you fix these errors?

Comment on lines 31 to 45
!titleInput.value.trim() ||
!authorInput.value.trim() ||
!pagesInput.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);
myLibrary.push(
new Book(
titleInput.value.trim(),
authorInput.value.trim(),
Number(pagesInput.value),
checkInput.checked
)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 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.

  • pagesInput.value could be a value like "3.1416".

Comment on lines 97 to 98
let deleteBtn = document.createElement("button");
deleteBtn.id = i + 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Variable name is still not consistent
  • Value assigned to the id attribute is not unique (when there are more than 5 books), and what is the use of the id attribute in this application?

@cjyuan 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 21, 2025
@MansoorM11 MansoorM11 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 21, 2025
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Changes look good. Well done.

@cjyuan 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 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Volunteer to add when work is complete and all review comments have been addressed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants