Skip to content

Conversation

fatmaevin
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

@fatmaevin fatmaevin 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

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.

@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
@fatmaevin
Copy link
Author

I tried to improve and adjust my code according to the feedback. Thank you.

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. I have few more suggestions.

Comment on lines 4 to 8
loadLibrary();
if(myLibrary.length===0){
populateStorage();

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation is off.

Comment on lines 55 to 58
titleInput.value = "";
authorInput.value = "";
pagesInput.value = "";
checkInput.checked = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also consider using .reset() method of a form element.

Comment on lines 62 to 64
function protectMyLibrary(){
localStorage.setItem("myLibrary" , JSON.stringify(myLibrary));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

May I suggest naming the function saveLibrary? Since “save” and “load” are usually paired, this would make the naming more consistent.

function loadLibrary() {
const storedLibrary = localStorage.getItem("myLibrary");
if (storedLibrary) {
myLibrary = JSON.parse(storedLibrary);
Copy link
Contributor

Choose a reason for hiding this comment

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

When a book is added to the array in submit() and populateStorage(), the code adds an instance of Book to the array.
On line 63, only the key–value pairs of the object are stored in the JSON-formatted string; the “prototype chain” of the Book object is not preserved. On line 69, the restored array therefore contains only plain objects, not Book instances.

In this app, it happens that a plain object works just as well as a Book instance.
In general, however, a more appropriate approach is to add code that converts the restored array into an array of Book instances.

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

cjyuan commented Aug 11, 2025

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 Reviewed Volunteer to add when completing a review with trainee action still to take. Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 11, 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