JavaScript front-end 'Library App' implementation from the Odin project

Question

I am currently working through the Javascript portion of the Odin Project curriculum and have completed the library app project. As far as I can see everything is working as expected and I have completed all the object requirements (users can add books to their library, delete books, and update the 'read' status of each book).

I am now looking for some help/feedback on my code so that I can improve for future projects. Specifically, I am looking for some advice on how I can implement the SOLID principles and OOP within the project, but any general feedback/do's and don'ts would be greatly appreciated as well.

Here is my Javascript code:

    let bookArray = [];

    class Book {
      constructor(title, author, numPages, status) {
        this.title = title;
        this.author = author;
        this.numPages = numPages;
        this.status = status;
      }
    }

    //adds each book that the user inputs into 'database'
    function addToLibrary(book) {
      bookArray.push(book);
    }

    //add new DOM element each time book is added
    function makeNewCard(array) {
      let card = document.createElement("div");
      let title = document.createElement("p");
      let author = document.createElement("p");
      let numPages = document.createElement("p");
      let statusBtn = document.createElement("button");
      let removeBookBtn = document.createElement("button");
      title.setAttribute("class", "title");
      author.setAttribute("class", "author");
      numPages.setAttribute("class", "numPages");
      statusBtn.setAttribute("class", "status");
      card.setAttribute("class", "book-card");
      removeBookBtn.setAttribute("class", "remove-book");
      //displays book array contents on the page
      for (let i = 0; i < array.length; i++) {
        title.textContent = array[i]["title"];
        author.textContent = array[i]["author"];
        numPages.textContent = array[i]["numPages"];
        statusBtn.textContent = array[i]["status"];
        removeBookBtn.textContent = "Delete";
        card.dataset.index = i;
      }
      statusBtn.textContent === "Read"
        ? (statusBtn.style.backgroundColor = "var(--read-book)")
        : (statusBtn.style.backgroundColor = "var(--unread-book)");
      statusBtn.addEventListener("click", (e) =>
        //function is called with event target
        updateStatus(e.target)
      );
      removeBookBtn.addEventListener("click", (e) => deleteBook(e.target));
      card.append(title, author, numPages, statusBtn, removeBookBtn);
      document.querySelector(".library-contents").appendChild(card);
    }

    const form = document.querySelector("form");
    const inputTitle = document.querySelector("#book-title");
    const inputAuthor = document.querySelector("#book-author");
    const inputNumPages = document.querySelector("input[type=number]");
    const checkBox = document.querySelector("#book-status");
    const addNewBtn = document.querySelector(".add-new");
    const overlay = document.querySelector(".overlay");
    //new book is added when form is submitted
    form.addEventListener("submit", function (e) {
      e.preventDefault();
      const book = new Book(
        inputTitle.value,
        inputAuthor.value,
        inputNumPages.value,
        checkBox.checked === true ? "Read" : "Unread"
      );
      addToLibrary(book);
      console.log(bookArray);
      makeNewCard(bookArray);
      form.classList.toggle("form-visible");
      overlay.classList.toggle("overlay-visible");
      return book;
    });

    function updateStatus(button) {
      const arrayPosition = parseInt(button.parentElement.dataset.index);
      const selectedBook = bookArray[arrayPosition];
      console.log(selectedBook);
      if (selectedBook.status === "Read") {
        selectedBook.status = "Unread";
        button.style.backgroundColor = "var(--unread-book)";
      } else {
        selectedBook.status = "Read";
        button.style.backgroundColor = "var(--read-book)";
      }
      console.log(selectedBook);
      button.textContent = selectedBook.status;
    }

    function deleteBook(deleteButtton) {
      const arrayPosition = parseInt(deleteButtton.parentElement.dataset.index);
      bookArray.splice(arrayPosition, 1);
      console.log(bookArray);
      const selectedCard = deleteButtton.parentElement;
      selectedCard.parentElement.removeChild(selectedCard);
      //make array from remaining cards on screen and reassign a data value according 
      to their position on screen
      let cards = document.querySelectorAll(".book-card");
      console.log(cards);

      for (let i = 0; i < cards.length; i++) {
        console.log(i);
        cards[i].dataset.index = i;
        console.log(cards[i]);
      }
    }

    addNewBtn.addEventListener("click", function () {
      form.classList.toggle("form-visible");
      overlay.classList.toggle("overlay-visible");
    });

HTML File
Link to repository
Live Project
This is my first time asking for a code review so I do apologize if I have missed out any important information!

Answer 1

Good job! This code is very clear, in large part thanks to consistent naming.

---

    function addToLibrary(book) {
      bookArray.push(book);
    }

You describe bookArray as a "database", which is fine. What strikes me here is that the order of calls to this function is very important. So it feels like bookArray is doing more than maintaining a "library" or "database" -- it feels like it is actually responsible for maintaining a display order, visible to the user. So we're conflating details of the Model with details of the View.

Consider taking the opportunity to alphabetize books by author or title here. The array list you chose is a perfectly good representation, but consider using an object mapping instead. (This critique is all about examining current requirements and anticipating likely new ones, rather than about "change this code to improve its correctness.")

---

    function makeNewCard(array) {

This is nice enough, but perhaps a little tediously long, enough that it might be worth the occasional extract method.

Or imagine you had an object which maps each of {card, title, author, numPages} to element & class attributes like {"p", "title"}. Then the three three repetitive stanzas of assignments might be a natural fit for some looping.

As an application is deployed and matures, I would expect that user feature requests would only cause the set of attributes to grow, e.g. {publisher, copyrightYear, coverArt}.

      statusBtn.textContent === "Read"
        ? (statusBtn.style.backgroundColor = "var(--read-book)")
        : (statusBtn.style.backgroundColor = "var(--unread-book)");

Here we are evaluating an expression strictly for side effects. I'm sure there are some folks who embrace that, and might even write it into their project's style guide, in which case more power to you. When I read it I find it slightly tricky, and would rather see an explicit if telling me that we're executing verbs rather than constructing some noun, some result expression value which it turns out gets discarded.

---

    const form = ...
    const inputNumPages = ...

I again wonder if having a mapping object handy would let us move at least some of these repetitive assignments into a loop. No biggie.

      addToLibrary(book);
      console.log(bookArray);
      makeNewCard(bookArray);

Kudos, this is lovely, keep it up! Having done the hard work of writing low-level routines which connect business domain concepts to JS primitives, now you can very concisely give this high-level description of what you want done.

The logging is perhaps a bit chatty? It makes perfect sense during initial authoring and debugging. But just before a review / PR submission is the perfect time to go back through them. Consider deleting obsolete debugs, or using a logging severity level that makes it easy to crank up verbosity a month from now whilst debugging some new issue.

In a few places, further down in the code, you log just an integer with no explanation of what it means or which routine logged it. If you do choose to keep those, consider logging some explanatory text along with it.

---

    function deleteBook(deleteButtton) {

It is perhaps slightly painful or inconvenient to delete a book? Consider using a set or mapping object as your "database", and decide if the resulting simplification of this function helps to motivate such a change. Or just keep it in mind for next time, when you're choosing between This or That datastructure for a new project.

      for (let i = 0; i < cards.length; i++) { ...
        cards[i].dataset.index = i;

Two minor concerns:

1. Renumbering after deletion is an internal complexity that stems from your datastructure choice, rather than coming from the business domain's requirements.
2. Currently this is a class invariant, a promise which holds true during the app's lifetime. Consider computing this as just a temp variable before the splice() deletion. And having done that, do we need such a renumbering operation at all?

---

This application appears to achieve its design goals.

I would be willing to delegate or accept maintenance tasks on this codebase.