Discussion on: Feedback on my JavaScript project

View post

Replies for: Congrats for getting the app out there! I haven't checked the source code yet (will do that later) but a suggestion would be to automatically jump...

just leaving some thoughts/opinions in an unordered list:

you use google analytics but don't have cookie banner telling users about this. read into GDPR compliance regarding collecting user data. Google: use google analytics gdpr compliant.
I'd move the <script> of your JS file to the end of <body>
I feel like you could reduce some of your .js files. For example the file setTheUI.js doesn't import anything or does anything fancy that it needs to be its own module. I'm sure you can find a different spot for it and get rid of the file entirely :).
Same goes for the function createBoxes in util.js. createBoxes is just a function in util but displayBoxes gets its own .js file? It's not all bad but just heavily inconsistent.
You use a lot of empty lines. If you use VsCode, look into the extension Prettier and set VsCode to format on save (can be found somewhere in the settings)

Example:

// pressletters.js

  // A lot of empty lines here

   // game over
   if (letters.length === 0) {

        // reset the state
        gameStarted = false

        button.disabled = false

        caption.innerHTML = 'Well done!'

        setTimeout(playSound, 500, audioGameOver)

    }    

    return gameStarted

}

// But here suddenly none
function playSound(sound) {
    sound.play()
}

This file could also be named more generic, something like userInput.js.

You're super optimistic that you always pass the correct variables to other functions for example:

import { newLetters, createBoxes } from './utils.js'

export function displayBoxes(letters, number) {

    // generate new letters
    letters = newLetters(number)

    // display boxes
    createBoxes(letters, game)

    return letters

}

At some point in here you should probably check if the input, letters and number are actually what they are supposed to be or if they even exist at all (aren't null/undefined). Something like

export function displayBoxes(letters, number) {
  if (!letters) return "oops...";
}

Some syntax "issues"

plus.addEventListener('click', () => {

    if (NEW_LETTERS < 6) { NEW_LETTERS++ }

    setnum.innerHTML = NEW_LETTERS

})

could be changed to

plus.addEventListener('click', () => {
    if (NEW_LETTERS < 6) NEW_LETTERS++;
    setnum.innerHTML = NEW_LETTERS;
})

You define NEW_LETTERS all in capslock probably because it's a global variable but the other globals aren't capitalized. Looking at script.js line 20 and 22.

Since the page is doing what you want it to do it's definitely not bad :) Especially not when just starting out, you did good. From here it's just a constant process of iterating through optimisations and cleanup. I'd start by installing some formatter like Prettier to get the overall format in every file the same. Then go over and think about how you can A: rename files to make them more generic and B: merge some files because you really don't need to create a new file for just a singular function if this function isn't accessed by several other modules.

Yar • Oct 5 '20

Thanks for an extensive review and encouragement!

That's superhelpful, I appreciate it!

Yar • Oct 5 '20

Hi! If there is room for one more question, here is where I'm stuck with this app 🙊

I started working in a single file. Then made attempt to break it into modules. And when I move those plus and minus event listeners to a separate file, I can't properly pass the NEW_LETTERS value back to the main app.

Created new branch to illustrate it github.com/ptifur/javascript-pract...

Perhaps you could guide me in the right direction 🤓

Alex Janke • Oct 5 '20

Easiest would be to write a function setNewLetters (choose whatever name) in your script.js that sets the value for you.

// in script.js
export function setNewLetters(newValue) {
  if (!newValue) return;
  newLetters = newValue;
}

So you can then call this function in your event listeners and update your variable this way. Skips passing values back and forth, back and forth.

Yar • Oct 5 '20

Thanks for the insight!

Didn't know you can call the functions both ways. Guess this will save me some time stumbling through tutorials 😜