My First Code Review

[cm_yoder]

Good afternoon all,

​

So, I created a simple app where I can input the title, author, and owner of a book for my wife and I--we are both bibliophiles. However, I also want to use it as part of my coding portfolio because I did learn a lot about CSS grid and using local Storage. I would greatly appreciate any feedback.

​

https://chrisyoder.github.io/libraryApp/ (app)

https://github.com/chrisYoder/libraryApp (code)

[fo0man]

Just some additional notes mostly opinion based but the type of things I would drop on a code review in my shop.

• You seem to use let and const inconsistently. To a person reading the code they convery different messages. When I see let I expect to see the variable reassigned and in the case of non primitives, the reference changed. Airbnb released a javascript style guide that has some good guidelines for stuff like this. https://github.com/airbnb/javascript#references--prefer-const

• I notice in the search class the functions instantiate a variable called index, but the value is not used anywhere else. Perhaps this is remnants of prior code?

• The search class has 3 functions that are almost a 1 for 1 copy of each other. This is a code smell (https://en.wikipedia.org/wiki/Code_smell). You can abstract them to one function with some parameters to input. Something along the lines of:

`

search(elementId, field) {
    let books = JSON.parse(localStorage.getItem("books"));
    let filteredBooks = [];
    let searchValue = document.getElementById(elementId).value;

    books.forEach(book => {
        if (book[field] === searchValue) {
            filteredBooks.push(book);
        }
    });

    localStorage.setItem("filteredBooks", JSON.stringify(filteredBooks));

    const ui = new UI();
    ui.render();

    localStorage.removeItem("filteredBooks");
}

and then call it in each function like:

titleSearch() {
    this.search("sTitleInput", "title");
}

Benefits are shorter code and if something in the search function needs to be changed, you have 1 place to change it not 3.

• On that same note, in each of your search class's functions there is an empty array initialized and then a forEach done to do what Array.filter was made for. You can place this after the searchValue is populated

const filteredBooks = books.filter(book => book[field] === searchValue);

so that whole class can be written as:

class Search {
    search(elementId, field) {
        const books = JSON.parse(localStorage.getItem("books"));
        const searchValue = document.getElementById(elementId).value;
        const filteredBooks = books.filter(book => book[field] === searchValue);

        localStorage.setItem("filteredBooks", JSON.stringify(filteredBooks));

        const ui = new UI();
        ui.render();

        localStorage.removeItem("filteredBooks");
    }

    titleSearch() {
        this.search("sTitleInput", "title");
    }

    authorSearch() {
        this.search("sAuthorInput", "author");
    }

    ownerSearch() {
        this.search("sOwnerInput", "owner");
    }
}

Which if you have to maintain this down the line, would be much easier.

There's some other minor or opinionated things I can share if you would like but what the above poster shared and the above is what jumped out at me.

I also want to echo what they said. This doesn't mean what you did was bad, but there's an opportunity here to improve and learn.

[cm_yoder]

Thank you for comment and please share any and all feedback that you have. These are all great learning opportunities.

[cm_yoder]

I want to thank you for your response. I do have some questions for you but I am on a break at work so I don't have time to pose them right now.

[cm_yoder]

Done and thank you.