Do you think my coding style is correct?

Code Feedback
1

Hello, I recently learned the API functions and developed a small movie site to try it myself. Since I am not very interested in the interface of the project, I want you to evaluate your comments in terms of javascript rather than this. Do you think I am using the OOP System correctly or what are the shortcomings in my code? Thank you for your feedback.

Github: Film-ProjectAPI/js at main · BerkayAkgurgen/Film-ProjectAPI · GitHub

Demo: Wond Film

2

Cool, it looks good. Here are some things that popped out to me as I scanned your code:

    searchInput.addEventListener('keydown', e => {
        let control = false
        if (e.which == 8 && searchInput.value.trim() == "") {
            control = true
        } else {
            control = false
        }
        if (control) {
            e.preventDefault()
            return
        }
    })

First of all, why initialize control if you are just going to set it to one or another value? You either don’t need to initialize control or you don’t need the else branch - they do the same thing. Furthermore, I would have just used a ternary:

    const control = e.which === 8 && searchInput.value.trim() === ""

Note the use of ===. And I would find a more descriptive name than “control”.

Also, looking at the logic, the whole thing could be reduced to

    searchInput.addEventListener('keydown', e => {
        if (e.which === 8 && searchInput.value.trim() === "") {
            e.preventDefault()
        }
    })

Not the lack of the return statement - there is no need to tell it to return here - it will do that when it gets the end of the function.

Things like this:

window.addEventListener('DOMContentLoaded', () => ui.clearInput())

can usually be shortened to:

window.addEventListener('DOMContentLoaded', ui.clearInput)

There is need to wrap the function call in an anonymous function just to keep the function from being called - just don’t call the function and pass the reference.

For this:

    if (target == "fa fa-star") {
        if (check.indexOf(event.target.parentElement.parentElement.parentElement.id) == -1) {
            Storage.addAllFilmsToStorage(event.target.parentElement.nextElementSibling.children[0].textContent, event.target.parentElement.parentElement.parentElement.id)
        }
    }

Why break apart those ifs? This is the same

    if (check.indexOf(event.target.parentElement.parentElement.parentElement.id) === -1 && target === "fa fa-star") {
      // ...

that matches the pattern of the next two. Speaking of which, since these are mutually exclusive, I’d want those in an if/else chain or a switch statement - once one is true, there is no need to check the others. I’d also store that check.index return value into a variable so I don’t need to call it for each check.

Again, here:

        let filmsID = film.map(item => {
            if (favoriteFilmsID.indexOf(item.attributes.id.textContent) == -1) {
                return
            } else {
                item.children[1].children[0].innerHTML = `
                <i class="fa fa-star" style="color:white" id="favorite"></i>
                `
            }
        })

First of all, why store filmsID - it never gets used? Secondly, isn’t this the same as:

        let filmsID = film.map(item => {
            if (favoriteFilmsID.indexOf(item.attributes.id.textContent) !== -1) {
                item.children[1].children[0].innerHTML = `
                <i class="fa fa-star" style="color:white" id="favorite"></i>
                `
            } 
        })

Note the use of === - don’t use == unless you specifically want implicit type coercion. But then I would ask why you’re using map here. The purpose of that is to create a new array from an old one. But you aren’t creating a new array here - you’re acting on each element of the array, which probably means you want to use forEach. True, you can use map in its place, but that has a very specific purpose so using it when you don’t need it can cause confusing code.

That is what I noticed. The modules looked logical to me, but I’m not really an OOP guy. I’m better at picking apart JS, so that’s what I did. I hope it helps.

2 Likes
3

Thanks it’s will be useful. Firstly ı can use the map because it’s very useful in this function but when ı use forEach it’s returned undefined and ı didn’t wanna work on it. Now I can see it looks ugly since I’ve always used long syntax, i’ll work on it.

4

I don’t understand what you mean about forEach returning undefined - you’re not returning anything meaningful here and you’re not doing anything with it anyway.

5

And let me make clear - your code looks good in general. This sort of “there’s a simpler way to do this” corrections are very common with learners. And this is one of the ways you learn it - by getting code reviews from more experienced coders. And for the record, I’m kind of nitpicky when it comes to this stuff.

6

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