Library project - javascript

Question

I am going through the Odin Project and have just finished the 'Library' project on their FullStack JavaScript course.
As this is the first time I'll have my code reviewed, I want to know what I'm doing wrong and what I could do better; I'm new to coding, but don't hold back.

This is the link to the Library Project instructions page

//array where the new book objects are stored
const myLibrary = [];

//makes the addBook button run the addBookToLibrary function
const addBook = document.querySelector(".addBook");
addBook.addEventListener("click", function (event) {
  event.preventDefault();
  addBookToLibrary();
});

function addBookToLibrary() {
  //object contrusctor function
  function Book(author, title, numOfPages) {
    this.author = author;
    this.title = title;
    this.numOfPages = numOfPages;
  }

  //variables containing the value of the input in the input boxes on the webpage
  const author = document.querySelector("#authorName").value;
  const title = document.querySelector("#bookTitle").value;
  const numOfPages = document.querySelector("#pageNum").value;

  //newBook variable makes a new object using object contructor
  //and the parameters of the new book are the same as the created variables above (input from user)
  const newBook = new Book(author, title, numOfPages);

  //stores the new book object inside the myLibrary array
  myLibrary.push(newBook);

  //Adds a book card as books get added
  const bookCards = document.querySelector(".bookCards");
  const newCard = bookCards.appendChild(document.createElement("div"));
  newCard.classList.add("card");

  //clears the input boxes
  const inputs = document.querySelectorAll("input");
  for (i = 0; i < inputs.length; i++) {
    inputs[i].value = "";
  }

  //takes last added book and then fill card with last book info
  myLibrary.forEach(function (item, index, array) {
    if (index === array.length - 1) {
      newCard.textContent = `Author name: ${author}`;
      newCard.textContent += ` | Book title: ${title}`;
      newCard.textContent += ` | Number of pages: ${numOfPages}`;
    }
  });

  //creates remove book button and read button to every card created
  const createRemove = document.createElement("button");
  createRemove.classList.add("remove");
  const createRead = document.createElement("button");
  createRead.classList.add("read");
  const cards = document.querySelectorAll(".card");
  cards.forEach(function (i) {
    i.appendChild(createRemove);
    i.appendChild(createRead);
  });

  //adds text to all remove buttons
  const removeBttn = document.querySelectorAll(".remove");
  removeBttn.forEach(function (i) {
    i.textContent = "Remove Book";
  });

  //adds text to all read buttons
  const readBttn = document.querySelectorAll(".read");
  readBttn.forEach(function (i) {
    i.textContent = "Read";
  });
  //removes the book card when clicking remove button on card
  removeBttn.forEach(function (i) {
    i.addEventListener("click", function () {
      i.parentElement.remove();
    });
  });

  //changes card color and shows message depending on if the user has read the book or not
  const bookMessage = document.createElement("p");
  readBttn.forEach(function (item, index, array) {
    item.addEventListener("click", function () {
      if (index === array.length - 1) {
        item.parentElement.appendChild(bookMessage);
        if (item.textContent === "Not read") {
          item.parentElement.setAttribute(
            "style",
            "background-color: lightcoral",
          );
          bookMessage.textContent = "✘ You have not read this book yet";
          item.textContent = "Read";
        } else if (item.textContent === "Read") {
          item.parentElement.setAttribute(
            "style",
            "background-color: lightskyblue",
          );
          bookMessage.textContent = "✓ You have read this book";
          item.textContent = "Not read";
        }
      }
    });
  });
}

.card {
    width: 300px;
    height: 200px;
    border: solid black 2px;
    text-align: center;
    font-size: 20px;
    display: flex;
    align-items: center;
    padding: 10px;
    flex-direction: column;
}

.bookCards {
    display: flex;
    flex-wrap: wrap;
    gap: 20px;
    padding: 10%;
    padding: 1%;
}

button {
    width: 105px;
    margin: 1px;
}

form {
    display: flex;
    gap: 5px;
    align-self: center;
    flex-wrap: wrap;
}

body {
    display: flex;
    flex-direction: column;
}

<!DOCTYPE html>
<html lang="en">
<head>
    <script src="script.js" defer></script>
    <link rel="stylesheet" href="styles.css">
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Library Project</title>
</head>
<body>
  <form>
    <label for="authorName">Author Name:</label><br>
    <input type="text" id="authorName" name="authorName"><br>
    <label for="bookTitle">Book Title:</label><br>
    <input type="text" id="bookTitle" name="bookTitle"><br>
    <label for="pageNum">Number of pages:</label><br>
    <input type="text" id="pageNum" name="pageNum"><br>
    <button class="addBook">Add new book</button>
  </form><br>
  <div class="bookCards"></div>
</body>
</html>

Answer 1

I was in the midst of writing an answer, but ggorlen beat me to it. I echo most of those recommendations. Some additional considerations:

• No need to nest the Book constructor function inside the addBookToLibrary function. This should be moved to the top level of the script.

• Use a class instead of a constructor function. Under the hood, they are the same thing, but a class is more idiomatic these days, although it is debatable whether this is an improvement.

• Clearing inputs can be a one-liner:

  document.querySelectorAll("input").forEach(input => input.value = '');
  

  The querySelectorAll function returns a NodeList object, which has a forEach function just like the Array class does.

• Even shorter, use the HTMLFormElement.reset() function:

  document.querySelector('form').reset();
  

  Does the same thing in this case.

• Get rid of unnecessary iteration over myLibrary to set new card info. Since you are setting info in the last card, and newCard is already the last card, just set the values.

  So this:

  //takes last added book and then fill card with last book info
  myLibrary.forEach(function (item, index, array) {
    if (index === array.length - 1) {
      newCard.textContent = `Author name: ${author}`;
      newCard.textContent += ` | Book title: ${title}`;
      newCard.textContent += ` | Number of pages: ${numOfPages}`;
    }
  });
  

  Can simply become:

  newCard.textContent = `Author name: ${author}`;
  newCard.textContent += ` | Book title: ${title}`;
  newCard.textContent += ` | Number of pages: ${numOfPages}`;
  

• Avoid re-renders when setting card content. I just noticed that newCard.textContent is being modified using the += operator. More than likely the browser is re-rendering that element (and re-flowing it) each time the = or += operator is used. Build up the text content in a variable first, then set the text content once:

  const content = `Author name: ${author}`
                + ` | Book title: ${title}`
                + ` | Number of pages: ${numOfPages}`;
  
  newCard.textContent = content;
  

  Arguably a one-liner for the variable is still plenty readable:

  const content = `Author name: ${author} | Book title: ${title} | Number of pages: ${numOfPages}`;
  
  newCard.textContent = content;
  

  The variable itself isn't so necessary given the line length:

  newCard.textContent = `Author name: ${author} | Book title: ${title} | Number of pages: ${numOfPages}`;
  

  Or if you really wanted to split it into multiplines:

  newCard.textContent = `Author name: ${author}`
                      + ` | Book title: ${title}`
                      + ` | Number of pages: ${numOfPages}`;
  

Answer 2

• Avoid pointless comments that just restate what the code clearly does:

  //array where the new book objects are stored
  const myLibrary = [];
  

• for (i = 0 creates a global, ripe for bugs and naming clashes. Always use let i here to scope the variable to the loop block.

• Use arrow functions rather than function, unless you need to bind this.

• Use class instead of function Book(author, title, numOfPages) { ... and move it to the outer scope.

• In your forEach callbacks, use the value e for your parameter, implying "element", not i, which implies "index". When you do use an index, the callback parameters will be .forEach((e, i) => {}).

• Prefer for..of to .forEach--the syntax is cleaner and it supports early returns. Only use .forEach when you need the index (or for (const [i, value] of someArray.entries()) as suggested in the comments).

• Prefer for..of to C-style counter loops.

• Use kebab-case, not camelCase in CSS. authorName will be author-name.

• Don't store state in the document like if (item.textContent === "Not read") {. Store it in JS, then reflect it one-way to the document, following a single source of truth principle. This is more extensible and not tied to a particular presentation scheme. Think in terms of model-view-controller, where the model and view are totally decoupled, and a controller layer handles events, extracts data from the model and reflects it to the view.

• Avoid monster functions. addBookToLibrary is too long--you can break it into sub-steps, probably each commented section. Once you break out a function, remove the comment, or convert it to a JSDoc. This is discussed in Kent Beck's Tidy First, but I don't have a rule reference handy (just read the whole book).

• The following code can simply be const lastBook = myLibrary.at(-1):

  myLibrary.forEach(function (item, index, array) {
    if (index === array.length - 1) {
  

  or just remove the loop and use newCard.

• Avoid prefixing variables with "my"--it's noisy, and Perl is over! Use simply library--we know it's your library. books is perhaps a clearer name.

• Watch out for deep nesting. You can flatten if (index === array.length - 1) { by inverting the condition and using an early return. This is one of the first things discussed in Tidy First.

This is a quick eye-ball review. I'll add a suggested rewrite if I have time later on.