Glasgow | 25-ITP-Sep |Mohammed Abdoon| Sprint 2 | book library #343
Conversation
M-Abdoon
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
There are some debugging code left in script.js. Can you remove it to keep the code clean?
debugging/book-library/script.js
Outdated
| let titleInput = document.getElementById("title").value; | ||
| let authorInput = document.getElementById("author").value.trim(); | ||
| const pagesInput = Number(document.getElementById("pages").value.trim()); | ||
| const checkBtn = document.getElementById("check"); |
There was a problem hiding this comment.
- Title may also contain leading/trailing space.
- The first three variables are assigned the input values (and not DOM elements). Why not do the same to the 4th variable?
- Note:
Number()ignores leading/trailing space in a string, sotrim()is optional on line 31.
There was a problem hiding this comment.
the first three variables store the current input values because we need them directly, while the checkBtn is only needed to attach a click event to it, not it's value. so that's why I handled it differently.
| titleInput = titleInput.replace(/</g, ""); | ||
| authorInput = authorInput.replace(/</g, ""); |
There was a problem hiding this comment.
What if a book title really contains a "<" character?
Why only remove '<' and not all "special characters" that may be misinterpreted as part of HTML code?
There was a problem hiding this comment.
I thought only '<' could be misinterpreted as part of HTML.
I removed the validation completely and used .textContent instead of .innerHTML when assigning the input to the new object, so the page is not affected when displaying the text.
| authorInput = authorInput.replace(/</g, ""); | ||
|
|
||
| console.log(titleInput); | ||
| if ( titleInput == "" || authorInput == "" || pagesInput == "" ) { |
There was a problem hiding this comment.
Why compare pagesInput to ""?
There was a problem hiding this comment.
To make sure whether the input isn't empty or not.
debugging/book-library/script.js
Outdated
| titleCell.innerHTML = myLibrary[i].title; | ||
| authorCell.innerHTML = myLibrary[i].author; |
There was a problem hiding this comment.
By assigning a value to .innerHTML, some "special characters" in the book title may be misinterpreted as part of HTML code.
There was a problem hiding this comment.
Yes. I used .textContent instead so we ignore any special characters.
debugging/book-library/script.js
Outdated
| let readStatus = ""; | ||
| if (myLibrary[i].check == false) { | ||
| if (myLibrary[i].check == true) { | ||
| readStatus = "Yes"; | ||
| } else { | ||
| readStatus = "No"; | ||
| } | ||
| changeBut.innerText = readStatus; | ||
| changeButton.innerText = readStatus; |
There was a problem hiding this comment.
It is possible to simplify the code on lines 83-89 into one or two lines of code. As a challenge, can you simplify it?
cjyuan
added
Reviewed
I just modified the code and made the required changes. Thank you. |
M-Abdoon
added
Needs Review
|
Changes look good. |
cjyuan
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
finished book library exercise.
Questions
No questions so far. Thank you.