Skip to content

Glasgow| 25-ITP-May |Bassam Alshujaa| Sprint 2 | book library #291

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Glasgow| 25-ITP-May |Bassam Alshujaa| Sprint 2 | book library #291

wants to merge 3 commits into from

Conversation

Bassam-Radwan
Copy link

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

I've debugged the script.js file

Questions

Ask any questions you have for your reviewer.

@Bassam-Radwan Bassam-Radwan added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 10, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Aug 10, 2025 •
edited
Loading

Have you tried looking up "How to change base branch in a PR on GitHub" ?

@Bassam-Radwan Bassam-Radwan changed the base branch from book-library to main August 10, 2025 17:15
@Bassam-Radwan
Copy link
Author

Thanks so much for your help, I've looked up to this and I edited it, Thanks again!

cjyuan
cjyuan reviewed Aug 10, 2025
View reviewed changes
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.

debugging/book-library/script.js Outdated
pages.value == ""
) {
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, title.value, parseInt(pages.value), check.checked);
Copy link
Contributor

Choose a reason for hiding this comment

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

  • The input string values could contain only space characters or leading/trailing space characters.
  • Page input could be negative or a string that resembles an invalid integer.

debugging/book-library/script.js Outdated
Comment on lines 42 to 46
document.querySelectorAll('.form-group input').forEach(input => {
if (input.type !== 'submit' && input.type !== 'checkbox') {
input.value = '';
}
});
Copy link
Contributor

@cjyuan cjyuan Aug 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  • Why not reset also the checkbox?

  • Could also consider using .reset() method of form.

debugging/book-library/script.js Outdated
Comment on lines 81 to 87
let readStatus = "";
if (myLibrary[i].check == false) {
if (myLibrary[i].check == true) {
readStatus = "Yes";
} else {
readStatus = "No";
}
changeBut.innerText = readStatus;
changeBut.textContent = 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 81–87 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 10, 2025
@Bassam-Radwan
Copy link
Author

Thanks for your feedback, I'll work on that.

@Bassam-Radwan Bassam-Radwan 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 11, 2025
@Bassam-Radwan
Copy link
Author

I've fixed the issues in html and improved the code in js file. Thank you for taking the time to review my work!

cjyuan
cjyuan reviewed Aug 11, 2025
View reviewed changes
debugging/book-library/script.js Outdated
Comment on lines 33 to 46
if (
titleInput.value.trim() == '' ||
authorInput.value.trim() == '' ||
pagesInput.value.trim() == ''
) {
alert("Please fill all fields!");
return false;
} else {
const pagesNum = Number(pagesInput.value.trim());
if (!Number.isInteger(pagesNum) || pagesNum <= 0) {
alert("Please enter a valid, positive number of pages.");
return false;
}
let book = new Book(titleInput.value, authorInput.value, parseInt(pagesInput.value), readCheckbox.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.

  • Please note that the raw title and author input could contain leading and trailing space characters.

debugging/book-library/script.js Outdated
Comment on lines 67 to 70
const rows = tbody.rows;
for (let i = rows.length - 1; i >= 0; i--) {
tbody.deleteRow(i);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We could clear the content of <tbody> using only one operation.

@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 11, 2025
@Bassam-Radwan
Copy link
Author

Thanks for your review, I've improved the inputs checking and used one line to remove thetbody. Thanks

@Bassam-Radwan Bassam-Radwan 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 12, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Aug 12, 2025

Changes look great. 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 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjyuan cjyuan cjyuan left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Complete Volunteer to add when work is complete and all review comments have been addressed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Bassam-Radwan @cjyuan