Anyone willing to review the JavaScript code I worked on a practice project?

const overlay = document.getElementById('overlay');

function toggleTabIndex(){
    const books = document.querySelectorAll('.book');
    books.forEach(book => {

        if (book.getAttribute('tabindex') === 0 || 
        book.getAttribute('tabindex') === null){

            book.setAttribute('tabindex', -1);
        }
        else {
            book.setAttribute('tabindex', 0);
        }
    });

    const links = document.querySelectorAll('a');
    links.forEach(link => {

        if (link.getAttribute('tabindex') === 0 || 
        link.getAttribute('tabindex') === null){

            link.setAttribute('tabindex', -1);
        }
        else {
            link.setAttribute('tabindex', 0);
        }
    });
}


const toggle = (() => {
    class Toggle{

        addBookModal(e){
            const addBookModal = document.getElementById('add-book-modal');
            addBookModal.classList.toggle('active');
            overlay.classList.toggle('active');

            toggleTabIndex();
        }
        bookInfoModal(e){
            if ((e.type == "keydown" && e.code == "Space") || (e.type == "keydown" && e.code == "Enter") || (e.type == "click")){
                const bookInfoModal = e.target.children[1];
                if (bookInfoModal == undefined) return 0;
                
                bookInfoModal.classList.add('active');
                overlay.classList.add('active');
                
                toggleTabIndex();
            }
            else return 0;
        }
    }

    return new Toggle();
})();

function closeModal(e){
    e.path[1].classList.toggle('active');
    overlay.classList.toggle('active');

    toggleTabIndex();
    
    if (e.path[1].children[4].children[1].children[1] != undefined){
        const saveChangeButton = e.path[1].children[4].children[1].children[1];
        saveChangeButton.classList.remove('active');

        for (let index = 0; index < book.collection.length; index++){
            if (e.path[1].getAttribute('id') !== book.collection[index].bTitle){
                continue;
            }
            else {
                if (e.path[1].children[4][0].value !== book.collection[index].bStatus){
                    e.path[1].children[4][0].value = book.collection[index].bStatus;
                }
            }
        }
    }

    
};

const buttons = (() => {
    const addBookButton = document.getElementById('add-book-btn');
    addBookButton.addEventListener('click', toggle.addBookModal);

    const closeButtons = document.querySelectorAll('.close-btn');
    closeButtons.forEach(button => {
            button.addEventListener('click', closeModal);
        });
})();

const book = (() => {
    class Book{
        constructor(){
            this.collection = [];
        }
        create(bTitle, bAuthor, bPages, bPic, bStatus){
            return {
                bTitle,
                bAuthor,
                bPages,
                bPic,
                bStatus,
            }
        }
        getData(e){
            e.preventDefault();

            const bTitle = addBookForm['title'].value;
            const bAuthor = addBookForm['author'].value;
            const bPages = addBookForm['pages'].value;
            let bPic = addBookForm['picture'].value;
            const bStatus = addBookForm['status'].value;

            if (bPic != '') bPic = URL.createObjectURL(addBookForm['picture'].files[0]);

            let newBook = book.create(bTitle, bAuthor, bPages, bPic, bStatus);
            book.collection.push(newBook);
            addBookForm.reset();
        
            toggle.addBookModal();
            book.displayInDOM(bTitle, bPic, bStatus);
            book.createInfo(bTitle, bAuthor, bPages, bStatus);
        
            overlay.classList.remove('active');
        }
        displayInDOM(bTitle, bPic, bStatus){
            const img = document.createElement('img');

            if (bPic == "") {
                img.setAttribute('src', 'img/no-cover.jpg');
                img.setAttribute('alt', bTitle);
            } else {
                img.setAttribute('src', bPic);
                img.setAttribute('alt', bTitle);
            }
        
            const newBook = document.createElement('div');
            newBook.setAttribute('title', bTitle);
            newBook.setAttribute('tabindex', 0);
            newBook.classList.add('book');
        
            if (bStatus == 'Read') newBook.classList.add('read');
            else if (bStatus == 'In Progress') newBook.classList.add('in-progress');
            else if (bStatus == 'Not Read') newBook.classList.add('not-read');
        
            newBook.appendChild(img);

            const bookRack = document.querySelector('.book-rack');
            const addBookButton = document.getElementById('add-book-btn');
            bookRack.insertBefore(newBook, addBookButton);
        
            console.log(book.collection);
        }
        createInfo(bTitle, bAuthor, bPages, bStatus){
            const bookInfoModal = document.querySelector('.book-info');
            const newBookInfo = bookInfoModal.cloneNode(true);
            newBookInfo.setAttribute('id', bTitle);

            const bookTitleElem = newBookInfo.children[0];
            bookTitleElem.textContent = bTitle;

            const bookAuthorElem = newBookInfo.children[2];
            bookAuthorElem.textContent = `By ${bAuthor}`;

            const bookPagesElem = newBookInfo.children[3];
            if (bookPagesElem.textContent <= 1) bookPagesElem.textContent = `${bPages} page in total`;
            else bookPagesElem.textContent = `${bPages} pages in total`

            const bookStatusElem = newBookInfo.children[4].children[0].children[0];
            bookStatusElem.value = bStatus;

            bookStatusElem.addEventListener('change', (e) => {
                const saveChangeButton = e.path[2][2];
                saveChangeButton.classList.add('active');
            })

            const bookRack = document.querySelector('.book-rack');
            for (const book of bookRack.children){
                if (book.getAttribute('title') !== bTitle){
                    continue;
                }
                else{
                    book.appendChild(newBookInfo);
                    book.addEventListener('click', toggle.bookInfoModal);
                    book.addEventListener('keydown', toggle.bookInfoModal);

                    const closeButtons = document.querySelectorAll('.close-btn');
                    closeButtons.forEach(button => {
                            button.addEventListener('click', closeModal);
                    })

                    const bookStatusForm = newBookInfo.children[4];
                    const deleteBookButton = newBookInfo.children[4][1];

                    deleteBookButton.addEventListener('click', deleteBook);
                    bookStatusForm.addEventListener('submit', saveChange);
                }
            }
        }
    }

    return new Book();
})();

const addBookForm = document.getElementById('add-book-form');
addBookForm.addEventListener('submit', book.getData);

function saveChange(e){
    e.preventDefault();
    const bTitle = e.path[1].getAttribute('id');
    const newBookStatus = e.target[0].value;

    const bookRack = document.querySelector('.book-rack');
    for (const book of bookRack.children){
        if(book.getAttribute('title') != bTitle){
            continue;
        }
        else {
            e.target[0].value = newBookStatus;
            if (newBookStatus === 'Read'){
                book.classList.remove('in-progress');
                book.classList.remove('not-read');
            }
            else if (newBookStatus === 'In Progress'){
                book.classList.add('in-progress');
                book.classList.remove('not-read');
            }
            else {
                book.classList.add('not-read');
                book.classList.remove('in-progress');
            }
        }
    }

    for (let index = 0; index < book.collection.length; index++){
        if (book.collection[index].bTitle != bTitle){
            continue;
        }
        else {
            book.collection[index].bStatus = newBookStatus;
        }
    }

    e.target[2].classList.remove('active');
    e.path[1].classList.remove('active');
    overlay.classList.remove('active');
}

function deleteBook(e){
    e.preventDefault();

    const bTitle = e.path[4].getAttribute('title');
    console.log(bTitle)
    const bookRack = document.querySelector('.book-rack');
    
    e.path[1].classList.remove('active');
    overlay.classList.remove('active');

    for (const book of bookRack.children){
        if (book.getAttribute('title') !== bTitle){
            continue;
        }
        else {
            book.remove();
        }
    }
    
    for (let index = 0; index < book.collection.length; index++){
        if (book.collection[index].bTitle !== bTitle){
            continue;
        }
        else {
            book.collection.splice(index, 1);
        }
    }
}

I refactored the JS code of my library project (above is the whole code snippet) as an exercise for the curriculum (The Odin Project) I’m currently focusing on. It tells me to replace my object constructors with classes (which I learned recently) but I took the step further to redo most of the code.

I will say, after finishing this, I realized that I could have improved on some parts like I could have used .querySelectorAll() in some areas for readability but what do you think?

Did I use JS classes right? Are there areas where you could have done better? If yes, can you tell me? Is this how actual object-oriented programming is done or?
You can take your time to analyze it, I am in no rush. I just want feedback for me to improve.

And below is the project itself which you can visit if you are interested…

So something like this would help at first:

const RemindApp = {
  selectors: {
    books: document.querySelectorAll(`data-remind="book"`),
    links: document.querySelectorAll(`data-remind="link"`),
    overlay: document.querySelector(`data-remind="overlay"`),
    addBookButton: document.querySelector(`data-remind="add-book"`),
    ...etc
}

Move the things you’re selecting to a single place, that way it’s much easier to find them and change things.

Also settle on one way of selecting things – I’ve used data attributes in the example for anything that is specifically used for selecting elements in the JS code, this is quite a common pattern. There’s nothing wrong with using classes and IDs, it’s just that classes are used to apply styling and tend to get changed frequently. IDs are a slight pain because they have to be unique, it’s easy to make mistakes.

Anyway end result is you can use whatever you want, because in the JS code you can then use like:

function toggleTabIndex() {
  RemindApp.selectors.books.forEach(book => {
    if (!book.getAttribute('tabindex') === 0) {
      ...dostuff

Then can also do something like

function toggleTabIndex (el) {
  const currentTabIndex = el.getAttribute("tabindex");
  el.setAttribute("tabindex", currentTabIndex ?  -1  :  0);
}

function  toggleTabIndicesOnAdd() {
    RemindApp.selectors.books.forEach(toggleTabIndex);
    RemindApp.selectors.links.forEach(toggleTabIndex);
}

I’d strongly advise not doing this:

e.path[1].children[4].children[1].children[1]

For one thing, it’s unreadable and impossible to understand without carefully looking at the HTML structure. But key issue is that if you change one thing in the HTML (like for example you need to wrap an element to make it easier to style), you’ll break all your code.

Select the element you want directly. If there are many on the page and they need to be unique, give each one a unique ID (either literally an ID, or a data attribute).

Don’t tie the JS code to an extremely specific HTML structure.


Why is everything wrapped in self-executing functions? So, for example:

const book = (() => {
  class Book {
    ...etc
  }
  return new Book();
})();

Just use a class? Like

class Book {
  ...do stuff
}

Then when you want to create a new one, just do new Book(.....some data to initialise it with). Or if you really want, you can add a method to do that

class Book {
  ...do stuff
   
  static create(...some init data) {
    return new Book(...some init data);
  }
}

Then when you want a new book, can do Book.create(...some init data)


Again, with the classes you’re tying things to an extremely specific HTML structure. Surely a “book” should just have data – if I create a new book, I would expect

new Book(
  "Ironopolis",
  "Glen James Brown",
  464,
  "https://cdn.shopify.com/s/files/1/1097/7086/products/Ironopolis_shortlist_cover_large.jpg?v=1578309952",
  "in-progress",
)

to construct something like:

{
  title: "Ironopolis",
  author: "Glen James Brown",
  pages: 464,
  status: "in-progress",
  pic: "https://cdn.shopify.com/s/files/1/1097/7086/products/Ironopolis_shortlist_cover_large.jpg?v=1578309952"
}

but it doesn’t, it does loads of other stuff.

If you’re using classes (you don’t need to but there’s no issue with that), you want something like

class Book // individual book data
class BookShelf // stores Books in an array or object or whatever

And they’re just data.

Then you can have render methods or just functions that read/update the data.

You can add a render (or whatever you want to call it) method to a class and have that be where you talk to the DOM, but don’t lose sight of the fact that a class is just some data with (optionally) some functions attached that act on that data.


EDIT: I’d strongly advise looking at this code for ideas: GitHub - 1Marc/todomvc-vanillajs-2022: TodoMVC with Modern (ES6+), Vanilla JavaScript, it’s by the CEO of the online learning platform frontend masters, and it’s pretty damn good (particularly as he put it together so fast) – here’s the supporting blog post as well. It’s around 170 lines of code, and it’s fully functional

1 Like

Dan hit on a lot of the high level stuff. Let me look at some lower, JS things:


    books.forEach(book => {

        if (book.getAttribute('tabindex') === 0 || 
        book.getAttribute('tabindex') === null){

            book.setAttribute('tabindex', -1);
        }
        else {
            book.setAttribute('tabindex', 0);
        }
    });

I would have done something like:

    books.forEach(book =>
      book.setAttribute('tabindex', book.getAttribute('tabindex') ? 0 : -1)
    )

And similarly for links and in other places.


        addBookModal(e){
            const addBookModal = document.getElementById('add-book-modal');
            addBookModal.classList.toggle('active');
            overlay.classList.toggle('active');

            toggleTabIndex();
        }

This method doesn’t use that parameter.


            else {
                if (e.path[1].children[4][0].value !== book.collection[index].bStatus){
                    e.path[1].children[4][0].value = book.collection[index].bStatus;
                }
            }

Is this different than:

            else {
                e.path[1].children[4][0].value = book.collection[index].bStatus;
            }

I don’t know, maybe fewer DOM updates.


        create(bTitle, bAuthor, bPages, bPic, bStatus){
            return {
                bTitle,
                bAuthor,
                bPages,
                bPic,
                bStatus,
            }
        }

Is this doing anything? Maybe it is scaffolding to do something later.


if (bPic != '') bPic = URL.createObjectURL(addBookForm['picture'].files[0]);

Is this different than:

if (!bPic) bPic = URL.createObjectURL(addBookForm['picture'].files[0]);

            if (bPic == "") {
                img.setAttribute('src', 'img/no-cover.jpg');
                img.setAttribute('alt', bTitle);
            } else {
                img.setAttribute('src', bPic);
                img.setAttribute('alt', bTitle);
            }

could be:

              img.setAttribute('src', bPic ? bPic :'img/no-cover.jpg');
              img.setAttribute('alt', bTitle);

            if (bStatus == 'Read') newBook.classList.add('read');
            else if (bStatus == 'In Progress') newBook.classList.add('in-progress');
            else if (bStatus == 'Not Read') newBook.classList.add('not-read');

I would prefer a switch statement here, but that’s getting subjective.


            for (const book of bookRack.children){
                if (book.getAttribute('title') !== bTitle){
                    continue;
                }
                else{
                    book.appendChild(newBookInfo);
                    book.addEventListener('click', toggle.bookInfoModal);
                    book.addEventListener('keydown', toggle.bookInfoModal);

                    const closeButtons = document.querySelectorAll('.close-btn');
                    closeButtons.forEach(button => {
                            button.addEventListener('click', closeModal);
                    })

                    const bookStatusForm = newBookInfo.children[4];
                    const deleteBookButton = newBookInfo.children[4][1];

                    deleteBookButton.addEventListener('click', deleteBook);
                    bookStatusForm.addEventListener('submit', saveChange);
                }
            }

avoid nesting when you can:

avoid nesting when you can:

            for (const book of bookRack.children){
                if (book.getAttribute('title') !== bTitle){
                    continue;
                }

                book.appendChild(newBookInfo);
                book.addEventListener('click', toggle.bookInfoModal);
                book.addEventListener('keydown', toggle.bookInfoModal);

                const closeButtons = document.querySelectorAll('.close-btn');
                closeButtons.forEach(button => {
                        button.addEventListener('click', closeModal);
                })

                const bookStatusForm = newBookInfo.children[4];
                const deleteBookButton = newBookInfo.children[4][1];

                deleteBookButton.addEventListener('click', deleteBook);
                bookStatusForm.addEventListener('submit', saveChange);
            }

here and in other places.


In general, avoid == and != - they can lead to unexpected results in some cases.

Also, the formatting is a little odd at times. I personally hate indenting 4 spaces, but that again is subjective. I would install a linter are prettier with a standard config and get used to working with those. In the real world, you’ll probably have formatting standards set for you, and it will probably be pretty close to one of those.

1 Like

Thank you @DanCouper and @kevinSmith for taking your time to review my code. The advice you both heed is exactly what I needed, and I’ll make sure to use it whenever I’m doing the next set of exercises and practice projects.

Can I ask? When do you usually use IIFE’s when writing JS on some of the projects you are working on? I can’t seem to think of any particular use of it…


Sure thing! I’ll make sure to dive in on how he implements JS and use that knowledge for the rest of the future projects I’ll be building.


It actually does, I think it could have been better but here it is:

if (bPic != '') bPic = URL.createObjectURL(addBookForm['picture'].files[0]);

let newBook = book.create(bTitle, bAuthor, bPages, bPic, bStatus);
book.collection.push(newBook);

Actually yes, what I tried to do is if the user changed the status of the book but did not save it and instead closed the modal, the value of the select element (the one that holds the status of the book) would revert back to its original value, which I took from the book.collection array.

And yeah, I’m sorry if this looked complex to you. I’ll make sure to limit or rather not use HTML structure paths as selectors for my elements.


I think so, I tested it. If I used what you made above, the bPic variable with the empty string '' would be assigned the URL object and its result, not the other way around.


Can you elaborate with that statement? You mean I should not nest conditional statements inside of loops or is there something I do not understand?


Ah, I see. I actually have prettier installed but rarely used it :sweat_smile:. I think I should use it more often then.

@DanCouper and @kevinSmith have given great feedback. I just want to contribute my opinion on one thing.

This is why I think it is best to use tabs for indenting instead of spaces. Then each person can configure their editor to use however many spaces they like for a tab and you aren’t forcing anyone to read code in a format they don’t like.

It actually does, I think it could have been better but here it is:

My point wasn’t that it doesn’t get used, just that it wasn’t doing anything useful. It’s just wrapping the params in an object. What does that accomplish? I mean, I guess if it’s part of a pattern it’s OK. It just caught my eye.

It actually does, I think it could have been better but here it is:

I got my logic backwards, it should have been if (bPic).... The point is that most devs would just do a truthy/falsy check there. 99 times out of 100, if that isn’t sufficient, then your data is set up wrong.

Can you elaborate with that statement? You mean I should not nest conditional statements inside of loops or is there something I do not understand?

Don’t nest where you don’t have to. If you have to nest, then nest. If you don’t, don’t. Nesting hampers readability. Rather than…

if (cond) {
  // do something
  return
} else {
  // do something
  // do something
  // do something
  // do something
  // do something
  // do something
  // do something
  // do something
  // do something
  return
}

I’d rather see:

if (cond) {
  // do something
  return
}

// do something
// do something
// do something
// do something
// do something
// do something
// do something
// do something
// do something
return

Often we have nests inside of nests. It can be confusing to visually keep track of all the layers. Always look for ways to eliminate layers by using a return early pattern like here, or pulling things out into a function, reordering logic, etc. One of the marks of a great coder is that their code is very readable. That doesn’t happen by accident.

Ah, I see. I actually have prettier installed but rarely used it :sweat_smile:. I think I should use it more often then.

A linter and prettier should be set up in your IDE. Prettier should run and format your code every time you save your file.

I’ve never really had a choice in the subject. At the three companies I’ve worked, the enforced standard was two spaces. Anything else got flagged by prettier and “fixed”.

My point is that if I see:

if (x !== y) {
  x = y
}

I wonder why they didn’t just go:

x = y

The end result is exactly the same. I don’t know, there might be some performance difference, but I think that 99.9999% of the time I think that micro-optimization is just self-gratification.

Well even though it was only used once (and the object is pushed to the book.collection array), it just does that. Nothing else really :person_shrugging:.

Ah, I understand now and I agree. Sometimes when I nest things, it can get confusing real quick if the names aren’t that descriptive or that I nested a bunch of statements inside conditions and another set of conditions…

I can’t remember explicitly writing an IIFE for years now*. I used to occasionally, before modern tools and modern additions to the language, mainly for creating modules. I’m not sure why you’re using them here

* Only thing I might have written one for IIRC is for some async intialisation logic (so it runs some async code when I start the program).

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.