London | 26-ITP-May | Damilola Odumosu | Sprint 2 | Book Library - #533
London | 26-ITP-May | Damilola Odumosu | Sprint 2 | Book Library#533d-odumosu wants to merge 1 commit into
Conversation
d-odumosu
added
Needs Review
| class="form-control" | ||
| id="pages" | ||
| name="pages" | ||
| min="1" | ||
| required | ||
| /> |
There was a problem hiding this comment.
Could you find out why the browser does not check if the value in this input field is a positive whole number (even if when the constraints min="1" is specified)?
| type="submit" | ||
| value="Submit" | ||
| class="btn btn-primary" | ||
| onclick="submit();" |
There was a problem hiding this comment.
Could you find out the tradeoff between
A) specifying a callback via an onclick attribute in HTML, and
B) specifying a callback in JavaScript
?
| @@ -1,103 +1,101 @@ | |||
| let myLibrary = []; | |||
There was a problem hiding this comment.
Can we declare myLibrary in a way that prevents it from being accidentally reassigned?
| let book = new Book( | ||
| titleInput.value, | ||
| authorInput.value, | ||
| Number(pagesInput.value), | ||
| checkInput.checked | ||
| ); |
There was a problem hiding this comment.
-
Is
Number(pagesInput.value)always a valid positive whole number? -
The code checks the trimmed value but it uses the raw values.
A better and safer approach to deal with user input is to first store the preprocessed input in variables, validate them, then use those cleaned values consistently throughout the rest of the code.
| const titleInput = document.getElementById("title"); | ||
| const authorInput = document.getElementById("author"); | ||
| const pagesInput = document.getElementById("pages"); | ||
| const checkInput = document.getElementById("check"); |
There was a problem hiding this comment.
Common practice is to declare all shared variables/constants at the beginning of the file before function definitions.
| }); | ||
| } | ||
|
|
||
| const tableBody = document.querySelector("tbody"); |
There was a problem hiding this comment.
Why not keep this variable declaration with the other variable declarations at the beginning of the file?
| } | ||
|
|
||
| const tableBody = document.querySelector("tbody"); | ||
| let readStatus; |
There was a problem hiding this comment.
Does readStatus need to be shared among functions? If not, it is better to declare it as a local variable in the function where it is used.
| if (!book.check) { | ||
| readStatus = "No"; | ||
| } else { | ||
| readStatus = "Yes"; | ||
| } | ||
| changeBtn.innerText = readStatus; |
There was a problem hiding this comment.
This could be a good opportunity to practice using the ? : conditional operator. Could you rewrite the code on lines 86–91 as a single statement?
| alert(`You've deleted title: ${book.title}`); | ||
| myLibrary.splice(index, 1); | ||
| render(); |
There was a problem hiding this comment.
The alert message is shown before the book is actually deleted; the deletion only occurs after the alert dialog is dismissed. This introduces a risk that the operation may not complete (e.g., if the user closes the browser before dismissing the alert).
In general, it’s better to display a confirmation message only after the associated operation has successfully completed.
In addition, alert() is a blocking function call. As a result, invoking it prevents the browser from updating the UI until the dialog is dismissed.
Suggestion: Research for approaches that allows the UI to update before displaying the alert dialog.
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist