JavaScript front end for Odin Project book library database

Question

I am currently working on The Odin Project and have reached the JavaScript Part. There is a project, to create a Website that serves a a library. You can add a book and it appears on the site as a tile/panel.

I have finished the project and as far as I can see it works fine. But I feel like my JavaScript code is extremely messy and not very effective at all.

I would really appreciate it, if someone could take the time to look it over and give me some "style" feedback. Or maybe some do's and dont's!

Here is my JavaScript code:

let myLibrary = []; //array for storing books, each book is an object

let counter = 0;

const title = document.querySelector("#title");
const author = document.querySelector("#author");
const pages = document.querySelector("#pages");
const checkbox = document.getElementById("read");

const error = document.querySelector(".error");

document.querySelector(".addBook").addEventListener("click", () => {
    document.querySelector(".popup").style.display = "flex";
});
document.querySelector(".close").addEventListener("click", () => {
    document.querySelector(".popup").style.display = "none";
})
document.querySelector(".submit").addEventListener("click", function(event) {

    event.preventDefault();
    
    //get input from fields.
    let bookTitle = title.value;
    let bookAuthor = author.value;
    let bookPages = pages.value;
    let bookRead ;
    if(checkbox.checked) {
        bookRead = Boolean(true);
    }
    else {
        bookRead = Boolean(false);
    }
    if(bookTitle.length > 0 && bookAuthor.length > 0 && bookPages.length > 0) {
        const book = new Book(bookTitle, bookAuthor, bookPages, bookRead);
        if(isInLibrary(book)) {
            error.textContent = "This book is already in your library";
        }
        else {
            addBookToLibrary(book);
            displayBookInLibrary(book);
            document.querySelector(".popup").style.display = "none";
            reset();
        }
    }
    else {
        error.textContent = "Fill out all fields";
    }
    changeColor();
    remove();
});

function reset() {
    title.value = null;
    author.value = null;
    pages.value = null;
    checkbox.checked = false;
    error.textContent = "";
}

function Book(title, author, pages, read) {
    this.title = title;
    this.author = author;
    this.pages = pages;
    this.read = Boolean(read);
}

function addBookToLibrary(book) {
    myLibrary.push(book);
}

function isInLibrary(book) {
    let includes = false;
    for(i = 0; i < myLibrary.length; i++) {
        if(myLibrary[i].title == book.title) {
            includes = true;
        }
        else {
            includes = false;
        }
    }
    return includes;
}

function displayBookInLibrary(book) {

    const bookContainer = document.querySelector("#bookContainer");
        //add book to grid.
        let div = document.createElement("div");
        let hasRead = "";
        let clr = "";
        if(book.read) {
            hasRead = "Read";
            clr ="green";
        }
        else {
            hasRead = "Not read yet";
            clr="red";
        }
        div.setAttribute("data-num", counter);

        div.style.width = "13rem";
        div.style.height = " 13rem";
        div.style.border = " 4px solid #c31";
        div.style.display = "flex";
        div.style.flexDirection = "column";
        div.style.gap = "10px";
        div.style.justifyContent = "center";
        div.style.alignItems = "center";
        div.style.textAlign = "center";
        div.style.borderRadius = "5px";
        div.innerHTML = `
        <p class="BookTitle">"${book.title}"</p>

        <p class="BookAuthor">${book.author}</p>

        <p class="BookPages">${book.pages}${" pages"}</p>

        <button style="background: ${clr}; color: white; padding: 5px; border-radius: 4px;" class="HasRead">${hasRead}</button>

        <button class="Remove">Remove</button>
        `;

        bookContainer.appendChild(div);
        book.dataNum = `${counter}`;

        counter++;
        console.log(book);
}

function changeColor() {
    //change color and text of read buttons
    const buttons = document.querySelectorAll(".HasRead").length;

    for(var i = 0; i < buttons; i++) {

        let currentBtn = document.querySelectorAll(".HasRead")[i];
        currentBtn.addEventListener("click", function() {
            if(currentBtn.textContent == "Read") {
                currentBtn.textContent = "Not read yet";
                currentBtn.style.background = "red";
            }
            else {
                currentBtn.textContent = "Read";
                currentBtn.style.background = "green";
            }
        })
    }
}

function remove() {

    //remove buttons:
    const buttonsRemove = document.querySelectorAll(".Remove").length;
    console.log(buttonsRemove);
    for(var i = 0; i < buttonsRemove; i++) {
        let currentBtn = document.querySelectorAll(".Remove")[i];
        currentBtn.addEventListener("click", function() {
            //get parent element attribute
            //remove said button from everything
            let parentAttr = currentBtn.parentNode.getAttribute("data-num");
            for(const book of myLibrary) {
                if(book.dataNum == parentAttr) {
                    const index = myLibrary.indexOf(book);
                    const div = document.querySelector(`div[data-num="${parentAttr}"]`);
                    document.getElementById("bookContainer").removeChild(div);

                    myLibrary.splice(index, 1);//remove said book from array
                }
                else {
                    return;
                }
            }
        });
    }
}

Here is a link to my Github repository with the whole project, should you be interested: https://github.com/morloq/Library.

Answer 1

There are of course many ways to structure the code, and its interesting that you opted to encapsulate Book but not Library. You might want to look into how OOP would help you group together related concepts and methods. e.g.

const library = new Library();
library.addBook(new Book(...));

You also aren't taking advantage of the standard library (as in API, not because JS already contains the concept of a book repository), and have some misunderstandings about Boolean types.

Use of Boolean(true|false)

This is unnecessary. The constants true and false are booleans, so you just need to assign them (e.g. bookRead = true). There is a place for wrapping a non-boolean (such as a counter) but in reality its rare as we can rely on non-zero, non-null, non-undefined values being interpretted as boolean depending on context. e.g.

let value = Math.random();
if (value) {
  // will be here for any non-zero value
} else {
  // will be here only for zero value

Because of this, most people will use double negation to convert a 'truthy' value to a boolean, e.g.:

let value = Math.random();
let bool = !!value;  // true for non-zero value

This misunderstanding is most apparent here:

if(checkbox.checked) {
  bookRead = Boolean(true);
} else {
  bookRead = Boolean(false);
}

Which can simply become:

bookRead = checkbox.checked;

This leads onto unnecessary verbosity when testing numeric values:

if(bookTitle.length > 0 && bookAuthor.length > 0 && bookPages.length > 0) { ...

This can be simplified to:

if(bookTitle.length && bookAuthor.length && bookPages.length) { ...

Sometimes it pays to be more explicit, but other times not.

isInLibrary

As mentioned above, you have a tendancy to long hand instead of leveraging the standard library methods.

myLibrary is an array, and Array has standard methods to determine whether an item is in a collection. Array.prototype.includes is not useful here, because we are not comparing the whole book (and object comparison is a separate topic worth understanding), but we can leverage Array.prototype.find.

Indeed, your implementation doesn't work because it compares every book title in the library, so will only return true when the target book is the last book. In the loop, you should return as soon as a match is found.

Leveraging the standard library, we can succinctly write:

function isInLibrary(book) {
  return myLibrary.find(item => item.title === book.title) !== undefined
}

NB: Generally, you should use strict equality (===) instead of == to avoid type conversions that lead to unintended results.

changeColor

Here is an example of where you are doing more work than necessary.

querySelectorAll returns a NodeList, which has a forEach method. In your implementation, you are querying multiple times, throwing away useful information, I think because you are focussing on the concept of for loops instead of iteration.

function changeColor() {
  document.querySelectorAll('.HasRead').forEach(button => {
    button.addEventListener('click', () => {
      button.textContent = button.textContent === 'Read' ? 'Not read yet' : 'Read';
      button.style.background = button.style.background === 'red' ? 'green' : 'red';
  });
}

I've not delved closely into the code, but you should check to make sure that you aren't repeatedly adding the same listener to DOM elements. You might not spot the duplication, but it'll add overhead to every event fired.

You can use this same style in the remove method.

remove

Using Array.prototype.splice in the for loop works in this instance, but there is a trap that you can fall into. Modifying the array whilst iterating over it will cause items to be skipped. There are ways around that (iterating backwards), but it's possible to filter and acheive the same result.

function remove() {

  document.querySelectorAll('.Remove').forEach(button => {
    button.addEventListener('click', () => {
      const dataNum = button.parentNode.getAttribute('data-num');
      myLibrary = myLibrary.filter((book, index) => {
        if (book.dataNum === dataNum) {
          const div = document.querySelector(`div[data-num="${dataNum}"]`);
          div.parentNode.removeChild(div);
          return false;
        }
        return true;
  });
}

There is a slight overhead here, in that filter is going to compare dataNum on all books, rather than returning after the first match. We return false for the match (the one we are removing) so that its filtered out of the new array assigned to myLibrary, and true for the non-matching ones we want to keep.

Assuming that bookContainer is the direct parent of item we are removing, we don't need to do another querySelector.

Answer 2

I will just go from "least" important to "most" important.

The code is easy to read, but there are some improvements you can do.

In this review, my code is to be seen as more of an example and/or a guideline, not as a final implementation.
Also, I'm using var opposed to const or let. Please use whichever you find appropriate.
You may want to read about it, a little more, to pick the best for you:
https://stackoverflow.com/questions/41086633/why-most-of-the-time-should-i-use-const-instead-of-let-in-javascript

---

Something I strongly recommend is to run the code in strict mode.

All you have to do is to add this line at the very top of your script: "use strict";.
This helps you to avoid novice mistakes, like assigning values to variables that don't exist.

Here's a simple example to show this:

function example_not_strict() {
  var count;
  cuont = 1; // spelling mistake
  // cuont is created globally
}

function example_strict() {
  "use strict";
  var count;
  contu = 1; // other spelling mistake
  // can't use cuont because it exists globally
  // silently created by running example_not_strict()
}

example_not_strict(); // no error
example_strict(); // ReferenceError

There are other changes, and I strongly recommend you to read the document about strict's mode:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode

---

You have the following block:

const title = document.querySelector("#title");
const author = document.querySelector("#author");
const pages = document.querySelector("#pages");
const checkbox = document.getElementById("read");

Why the mix of document.getElementById and document.querySelector?
In this case, I would pick document.getElementById, as it doesn't have to go through the DOM to find the elements.

---

There's so much CSS inside the JavaScript.
You should seriously consider moving it into a CSS stylesheet.

---

function displayBookInLibrary()

Following on the previous point, you have this function:

function displayBookInLibrary(book) {

    const bookContainer = document.querySelector("#bookContainer");
        //add book to grid.
        let div = document.createElement("div");
        let hasRead = "";
        let clr = "";
        if(book.read) {
            hasRead = "Read";
            clr ="green";
        }
        else {
            hasRead = "Not read yet";
            clr="red";
        }
        div.setAttribute("data-num", counter);

        div.style.width = "13rem";
        div.style.height = " 13rem";
        div.style.border = " 4px solid #c31";
        div.style.display = "flex";
        div.style.flexDirection = "column";
        div.style.gap = "10px";
        div.style.justifyContent = "center";
        div.style.alignItems = "center";
        div.style.textAlign = "center";
        div.style.borderRadius = "5px";
        div.innerHTML = `
        <p class="BookTitle">"${book.title}"</p>

        <p class="BookAuthor">${book.author}</p>

        <p class="BookPages">${book.pages}${" pages"}</p>

        <button style="background: ${clr}; color: white; padding: 5px; border-radius: 4px;" class="HasRead">${hasRead}</button>

        <button class="Remove">Remove</button>
        `;

        bookContainer.appendChild(div);
        book.dataNum = `${counter}`;

        counter++;
        console.log(book);
}

Instead of all this, you can do these steps:

• Add the following to your stylesheet:

.Book {
    width: 13rem;
    height: 13rem;
    border: 4px solid #3c1;
    display: flex;
    flex-direction: column;
    gap: 10px;
    justify-content: center;
    align-items: center;
    text-align: center;
    border-radius: 5px;
}

.Book .hasRead {
    background: red;
    color: white;
    padding: 5px;
    border-radius: 4px;
}

.Book[data-read="true"] .HasRead {
    background: green;
}

• Create the following <template>:

<template id="newBook">
    <div class="Book" data-num="" data-read="false">
        <p class="BookTitle"></p>

        <p class="BookAuthor"></p>

        <p class="BookPages"></p>

        <button class="HasRead">Not read yet</button>

        <button class="Remove">Remove</button>
    </div>
</template>

• Get access to the contents of the template:

var template = document.getElementById("newBook");
var clone = template.content.cloneNode(true);

• Do all your HTML manipulations:

var div = clone.querySelector(".Book");
div.setAttribute("data-num", count);

if(book.read) {
    div.setAttribute("data-read", "true");
    var hasread = div.querySelector(".HasRead");
    hasread.textContent = "Read";
}

var title = div.querySelector(".BookTitle");
title.textContent = "\"" + book.title + "\"";

var author = div.querySelector(".BookAuthor");
author.textContent = book.author;

var pages = div.querySelector(".BookPages");
pages.textContent = book.pages + " pages";

bookContainer.appendChild(clone);

This should give the exact same result, with some points in favor:

• Everything is divided as it should be.
  The HTML stays inside the HTML file.
  The CSS stays inside the CSS styles.
  You don't have to be looking inside a JavaScript file to change some HTML and CSS anymore. Everything in it's place.
• A lot safer.
  What if, somehow, you end up with a book called All about the <script> tag, for example?
  Your code will render a <script> tag in the HTML, which is BAD.
• More flexibility and control.
  For example, if you want, you can change your code to write "page" instead of "pages", for books with only 1 page.
  This can be done inside a template string, but it is a lot more fiddly and unreadable.
  Or, if you want to support different quotes, for different languages, you can add support for the <q> element, by changing <p class="BookTitle"></p> to <p><q class="BookTitle"></q></p>.

---

Everything said in the review of this function can be applied to all over the JavaScript code submitted for review.

For example, these lines:

document.querySelector(".addBook").addEventListener("click", () => {
    document.querySelector(".popup").style.display = "flex";
});
document.querySelector(".close").addEventListener("click", () => {
    document.querySelector(".popup").style.display = "none";
})

If you change them to manipulate class changes, you can just do this:

const popup = document.querySelector(".popup");
document.querySelector(".addBook").addEventListener("click", () => {
    popup.classList.add("d-none"); // or any other class with display: none
});
document.querySelector(".close").addEventListener("click", () => {
    popup.classList.remove("d-none");
})

---

A very strong recommendation:
DO NOT add click handlers on form submit buttons.

Imagine that I use a keyboard to navigate, because of a physical limitation or because of a preference.
Or I simply prefer to press ⏎ Enter instead of clicking a button with the mouse.
Your whole process falls apart and the form is submitted anyway, which you didn't intent.

Instead, add an event listener to the <form> where the button is.

Instead of this:

document.querySelector(".submit").addEventListener("click", function(event) {
    event.preventDefault();
    // [...]
});

You can do this (using the changes from before):

popup.querySelector(".form").addEventListener("submit", function(event) {
    event.preventDefault();
    // [...]
});

And also, change your """.submit""" button into an actual submit button:

<button type="submit">Submit</button>

---

function reset()

Going from the changes above, this function isn't necessary at all.

Instead of having all this:

function reset() {
    title.value = null;
    author.value = null;
    pages.value = null;
    checkbox.checked = false;
    error.textContent = "";
}

You can do just this:

var form = popup.querySelector(".form");

[...]

form.reset(); // <-- resets the form to it's default values

---

You're also validating the input with this line:

if(bookTitle.length > 0 && bookAuthor.length > 0 && bookPages.length > 0) {
    [...]
}

Dave Meehan's answer shows how to improve this, but, there's other things that you don't take into account.

For example, what if the title is nothing but whitespace? Or what if the book has -300 pages?
The code accepts these values, without issues.

Here's a basic attempt at better validation:

// .trim() removes whitespace from the beginning and end of the string
var bookTitle = title.value.trim();
var bookAuthor = author.value.trim();

// converts the number to a base10 integer, or NaN
var bookPages = parseInt(pages.value, 10);

if(!bookTitle.length || !bookAuthor.length)
{
    // write your code to show a message saying that fields are empty
    [...]
}
else if(isNaN(bookPages) || bookPages < 1)
{
    // write your code to show that the bookPages is invalid
    [...]
}

// proceed handling the code normally
[...]

I've inverted the logic so it reads closer to natural language, and to make it easier to do early returns and reduce the number of brackets.

Another possibility is to add HTML5 input validation as well, which will prevent the form from submitting until all errors are cleared.
You can read about it on: https://developer.mozilla.org/en-US/docs/Learn/Forms/Form_validation

As an aside, if you intend to send this data to a server, at a later stage, you MUST repeat this validation on the server as well.

---

As an additional aside, you might want to wrap your whole code in a new scope (for example, a new function), like this:

(function(){
    // your code comes here
    [...]
})();

This will keep your variables from being created on the global scope, which is where your code is running currently.

If you don't like this, you can wrap your code inside an event handler, for either the load or DOMContentLoaded events:

window.addEventListener('load', function(){
    // your code comes here
    [...]
});

This allows you to use the async or defer attributes, without causing issues, which can accelerate the loading times of a page.

Alternatively, you may want to look into JavaScript modules, but probably could be left for a later time.

---

---

I've seen a little bit of discussion regarding event listeners in the comments of Dave Meehan's answer.

One idea is to do the "poor man's" version of jQuery's .on() method.

The idea is simple: Add the event handler to the top-most common parent, and then check if the target is what you expect.
This is not the best idea, but it is an idea.

An example to handle the clicks on the delete button from the book, could be something like this:

// all books are added to bookContainer
bookContainer.addEventListener("click", function(event){
    // event.target should be the element that the click event is targetting
    if(event.target
        || event.target.className !== "BUTTON"
        || event.target.classList.contains("Remove")
    ) {
        // drop out of the function if it isn't the element we want
        return;
    }
    
    event.preventDefault();
    // we should stop the click event from going down the DOM
    event.stopPropagation();

    // the button's parent is the .Book <div>
    var div = event.target.parentNode;

    // always provide a base to parseInt
    var number = parseInt(div.getAttribute("data-num"), 10);
    bookContainer.removeChild(div);

    // taken from Dave Meehan's answer
    myLibrary = myLibrary.filter((book, index) => book.dataNum === number);
    // alternatively, stick to the .splice method, it works the same here
});

It's a bit of an odd pattern, but does exactly what we want: detects when a "click" event was triggered on a button with the class "Remove", and does what it needs to.
For everything else, the event keeps going as if nothing happened.