Refactoring the original implementation by hora · Pull Request #1 · hora/Book-Finder-App

hora · 2021-01-10T19:32:58Z

This PR (pull request) compares the original branch written by my student @IrvHenri, and suggested changes in the refactored branch written by me.

All non-trivial suggested changes, and some cosmetic changes, have line notes directly on the diff. You can see the conversations about those changes below, or in the 'Files changed' tab.

hora · 2021-01-10T19:39:33Z

index.html

-            id="searchInput"
-            name="searchInput"
+            id="search-input"
+            name="search-input"


For consistency with other class and ID names, the camelCase searchInput would be better as the kebab-case search-input.

hora · 2021-01-10T19:41:01Z

index.html

            type="text"
            aria-label="book search input field"
-            placeholder="Type author, book, name, subject..."
+            placeholder="Search by author, book, name, subject..."


I think the UX is a bit clearer with a more descriptive verb.

hora · 2021-01-10T19:47:25Z

index.html

-        >
+        <a href="https://github.com/IrvHenri" target="_blank">
+          Irving Henriquez
+        </a>


This just looks cleaner to me 😄

hora · 2021-01-10T19:51:50Z

script.js

-let searchForm = document.getElementById("search-form");
-
+const msgWaiting = document.querySelector(".message-waiting");
+const searchForm = document.getElementById("search-form");


For consistency, I changed all lets to consts in the set-up section of the script. A rule of thumb for some is that const should be the first choice, using let only when we're working with variables that we know are going to change. I'm not personally so strict about this, but I do like consistency. That's because inconsistency makes me ask why – could it be the result of a mistake? Could it cause a bug?

hora · 2021-01-10T20:00:22Z

script.js

-
-let apiKey = "&key=AIzaSyCw6aeDwd2srAlaiVBFSSBvyl2O7atCTaA";
+const googleBooksURL = "https://www.googleapis.com/books/v1/volumes?q=";
+const apiKey = "&key=AIzaSyCw6aeDwd2srAlaiVBFSSBvyl2O7atCTaA";


Exposing an API key to the public can be dangerous. You sometimes hear stories of API keys being stolen & used extensively, leading to giant unexpected (and unwanted) usage bills.

It's not very easy hiding a key when you're working with client-side code, though. Even if you hid the key from GitHub (for example by using environment variables), your API key would be exposed in the developer tools – anyone could in theory look at the network tab to find it.

So, when we're thinking about security, the question isn't how to we eliminate risk, but how much risk are we comfortable with? And what can we do to lower it to an acceptable level?

I recommend you set up HTTP restrictions on the Google API side. You should only allow requests using this API key from your GitHub pages URL once your site is deployed. (For ease of use, during development you can remove those restrictions but it's strongly recommended you turn them on once you've deployed your work).

hora · 2021-01-10T20:04:21Z

script.js

-let getGoogleBooks = "https://www.googleapis.com/books/v1/volumes?q=";
-
-let apiKey = "&key=AIzaSyCw6aeDwd2srAlaiVBFSSBvyl2O7atCTaA";
+const googleBooksURL = "https://www.googleapis.com/books/v1/volumes?q=";


A rule of thumb is to name functions as verbs because they do stuff, and variables as nouns because they are stuff. getGoogleBooks therefore sounded like a function, not a string which represents a URL.

hora · 2021-01-10T20:05:06Z

script.js

-       </div>
-        <div class = "book-details">
+  const searchInput = document.getElementById("search-input").value;
+  const searchQuery = `${googleBooksURL}${searchInput}${apiKey}`;


This variable doesn't contain any search results yet, it's just the completed query that's ready to be sent to the API.

hora · 2021-01-10T20:07:28Z

script.js

+  getBooks(searchQuery);
+});
+
+async function getBooks(query) {


It's better to move this function out of the submit event handler. That's because your event handler fires every time someone submits the form, which means your code was also re-defining getBooks every time the form was submitted.

Because the function is now outside the event handler, however, we need to pass it the search query.

hora · 2021-01-10T20:10:00Z

script.js

+  displayBooks(booksData.items);
+}
+
+function displayBooks(books) {


I also think the code is easier to follow when we break up the code for building the DOM elements with the book results into its own function.

hora · 2021-01-10T20:19:05Z

script.js

+  books.forEach((book) => {
+
+    let volumeInfo = book.volumeInfo;
+    let title = volumeInfo.title || "N/A";


I noticed that some search results don't contain data for all volumeInfo fields, and it was therefore showing up in the displayed results as "undefined," which would confuse non-web-developer users, and web developers would think it's a bug.

What this code does is ensure that if a volumeInfo field is undefined, that the text we show in the DOM is "N/A".

And how does this work? JavaScript executes Boolean expressions left to right. This means that A || B would be evaluated as follows:

If A is truthy, then A || B will also be truthy regardless of what B is. JavaScript will therefore not even bother to evaluate B.

If A if falsey, then A || B will evaluate to whatever B is, whether that's truthy or falsey. So the result of the expression A || B when A is falsey, is simply B.

We can therefore take advantage of the fact that undefined is falsey in JavaScript (as are a few other values – it's good to know what they all are).

So, if for example volumeInfo.title is undefined, then the value of title will be set to "N/A".
If volumeInfo.title is "Moon of the Crusted Snow", then the value of title will be set to "Moon of the Crusted Snow".

Note: For testing purposes, try searching for 'Moon of the Crusted Snow' - some of the search results have incomplete information, hence how I noticed this particular bug.

hora · 2021-01-10T20:21:01Z

script.js

+    let previewLink = volumeInfo.previewLink || "N/A";
+    let publishdate = volumeInfo.publishedDate || "N/A";
+    let publisher = volumeInfo.publisher || "N/A";
+    let smallThumbnail = getThumbnail(volumeInfo);


If the previous fields were undefined, we can easily set them to "N/A" and move on. But what if an image field is undefined?

This is a bit trickier, which is why I moved it into its own function. It's not much code, so that could could probably just live here, but I thought it was easier to scan when all the data we want is being set in a short line of code.

See the comment below for what getThumbnail does.

hora · 2021-01-10T20:29:21Z

script.js

+  });
+}
+
+function getThumbnail(volumeInfo) {


Here, we need to return a string with the URL to the book's thumbnail image. However, not all books have one.

This means we need to first check whether the returned search result has imageLinks and a smallThumbnail field. If it does, we can return the smallThumbnail field.

If it doesn't, there are several options. Because I didn't wanna look for a more appropriate thumbnail (like, for example, a generic book icon), I just return a Picsum (like Lorem Ipsum but for images). This will return a random image.

I don't actually like this solution because you have no idea what that random image is going to be, and I wouldn't particularly trust Picsum not to return something offensive or inappropriate. So I suggest you find a free to use book icon you can use in this situation, or come up with another solution that won't break your design when a search result doesn't have a thumbnail.

Note: For reference, on Jan 10, 2021, the 3rd result when searching for 'Moon of the Crusted Snow' doesn't have a thumbnail.

hora · 2021-01-10T20:31:20Z

Closing so the PR isn't accidentally merged.

hora · 2021-01-12T18:05:15Z

Actually, gonna keep this open for more visibility.

refactors original implementation

715b886

hora commented Jan 10, 2021

View reviewed changes

index.html

>

<a href="https://github.com/IrvHenri" target="_blank">

Irving Henriquez

</a>

Copy link

Owner Author

hora Jan 10, 2021

Choose a reason for hiding this comment

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

This just looks cleaner to me 😄

hora commented Jan 10, 2021

View reviewed changes

hora closed this Jan 10, 2021

zmrzl and others added 3 commits January 10, 2021 16:23

updates readme

40d52c4

updates readme

b273f34

fixes indentation

c72363e

hora reopened this Jan 12, 2021

updates readme

69214b9

Conversation

hora commented Jan 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hora Jan 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hora Jan 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hora commented Jan 10, 2021

Uh oh!

hora commented Jan 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

hora commented Jan 10, 2021 •

edited

Loading

hora Jan 10, 2021 •

edited

Loading

hora Jan 10, 2021 •

edited

Loading