Manchester | 25-ITP-Sep | Mahtem T. Mengstu | Sprint 2 | Book_library#353
Manchester | 25-ITP-Sep | Mahtem T. Mengstu | Sprint 2 | Book_library#353Mahtem wants to merge 5 commits intoCodeYourFuture:mainfrom
Conversation
Mahtem
added
the
Needs Review
This comment was marked as resolved.
This comment was marked as resolved.
github-actions
Bot
removed
the
Needs Review
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as duplicate.
This comment was marked as duplicate.
Mahtem
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
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.
| // Use IntelliSense to learn about possible attributes. | ||
| // Hover to view descriptions of existing attributes. | ||
| // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387 |
There was a problem hiding this comment.
-
JSON != JS. A JSON string containing JS-style comments is considered invalid.
-
Better to use
.gitignoreto exclude non-essential configuration files from being added to a repo.
There was a problem hiding this comment.
JSON != JS. A JSON string containing JS-style comments is considered invalid.
- Better to use
.gitignoreto exclude non-essential configuration files from being added to a repo.
Thank you, I will see what it really is. It happened accidentally.
| const lettersOnly = /^[A-Za-z\s]+$/; | ||
|
|
||
| // Title: letters + numbers + spaces (no special characters) | ||
| const titleAllowed = /^[A-Za-z0-9\s]+$/; |
There was a problem hiding this comment.
These two rules could exclude quite some real-world books.
There was a problem hiding this comment.
These two rules could exclude quite some real-world books.
Thank you, It has now been updated to handle wide range of author and title inputs normally.
| } | ||
|
|
||
| // Create and save the book | ||
| let book = new Book(title.value, author.value, pages.value, check.checked); |
There was a problem hiding this comment.
These raw values are not the values you validated. Consider
Step 1: Store preprocessed raw input in some variables
Step 2: Validate the values in the variables
Step 3: Refer to the values of the variables (to avoid accidently using raw input)
Also, some invalid "number of pages" can still pass the current validation rule.
There was a problem hiding this comment.
These raw values are not the values you validated. Consider Step 1: Store preprocessed raw input in some variables Step 2: Validate the values in the variables Step 3: Refer to the values of the variables (to avoid accidently using raw input)
Also, some invalid "number of pages" can still pass the current validation rule.
Thank you @cjyuan, the code has been updated to preprocess and validate all input values before storing.
cjyuan
added
Reviewed
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
The input validation is strong now.
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
…iptive names have been made.
Thank you @cjyuan for the constructive and comprehensive feedback I have included modifications from the guide, both in the index.html and script.js |
Mahtem
added
the
Needs Review
cjyuan
added
Complete
3 participants
Learners, PR Template
Self checklist
Changelist
Bugs in the book library fixed.
Questions
Questions will be asked in cyf_reviewers slack page.