Kevin Powell - Community•14mo ago

Rock, Paper, Scissors (Partial Solution)

Hello, I'm attempting to create the game "Rock, Paper, Scissors" using the files provided by FrontendMentor. My current solution logic somewhat works - https://codepen.io/Matt-CopOffMatt/pen/WNPbjqb Although this is probably not the optimal solution, I was wondering if anyone out there could check out my JS and give me some suggestions. Also, the only portion not working is:

    resetGame.addEventListener("click", () => {
        main.innerHTML = ""
        userOptions.classList.remove("hidden")
        userOptions.classList.added("visible")
    })

    resetGame.addEventListener("click", () => {
        main.innerHTML = ""
        userOptions.classList.remove("hidden")
        userOptions.classList.added("visible")
    })

which I'm confused about, as I'm adding display: none to an element, then removing, and adding display: block Let me know your thoughts (and yes, I know it's not done 👍 ) (Reference) - https://www.frontendmentor.io/challenges/rock-paper-scissors-game-pTgwgvgH

11 Replies

MattOP•14mo ago

Blue = Paper Yellow = Scissors Red = Rock

Joao•14mo ago

The problem is that you are overwriting the content of the main tag, so you cannot go back to what you had. You must store that value before you display the result of the game, although from what I'mseein at a glance there must be some entangling with other parts of the code. What I'd encourage is to break down the "findWinner" function which currently does a lot more than just finding the winner, it also adds and removes elements from the dom, keeps track of event listeners, etc. Something else that I think would help is to select all your DOM elements at the beginning of the file. Just get that out of the way and make it easy to look them up if needed. And be careful with event listeners, you can actually create memory leaks. For example you are creating one event listener inside the findwinner function, but on an element that is meant to be destroyed soon after, and then be re-created again, and again. You will end up with a bunch of dangling listeners that just stay there consuming memory for no reason. One easy fix for that is to provide a third parameter to addEventListener of { once: true } which you can imagine what it does, just be mindful of these things. This is also another reason why the reset doesn't work: you are creating and selecting elements and then using them inside a callback that assumes they will exist but by then, they've been overwritten ie., removed from the DOM. This may also be a good opportunity to look into the template element to reuse the same element inside the DOM programatically.

MattOP•14mo ago

Thanks for your feedback Adding this as third parameter doesn't fix the issue with listening for reset click

let resetGame = document.querySelector("#reset")

resetGame.addEventListener("click", function( {once: true} ) {
    main.innerHTML = ""
    userOptions.classList.remove("hidden")
    userOptions.classList.added("visible")
})

let resetGame = document.querySelector("#reset")

resetGame.addEventListener("click", function( {once: true} ) {
    main.innerHTML = ""
    userOptions.classList.remove("hidden")
    userOptions.classList.added("visible")
})

My current problem with this, is that when added outside of main function, it results in error beause #reset is non-existent

Chris•14mo ago

The options should be the 3rd parameter of addEventListener so it would look something like this:

resetGame.addEventListsener("click", () => {...}, { once: true })

resetGame.addEventListsener("click", () => {...}, { once: true })

Chris•14mo ago

https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#once

EventTarget: addEventListener() method - Web APIs | MDN

The addEventListener() method of the EventTarget interface sets up a function that will be called whenever the specified event is delivered to the target.

MattOP•14mo ago

I'm having trouble implementing this. I understand that the option once, if set true, only runs the listener once...however, how do I enable / disable to run it multiple times? I'm trying to work in this solution for:

buttons.forEach(btn => {
    btn.addEventListener("click", startGame)
});

buttons.forEach(btn => {
    btn.addEventListener("click", startGame)
});

as once the buttons are removed from DOM and re-added, they need the eventlistener again

Joao•14mo ago

Careful with that, you are not re adding them. You are creating new ones Therefore you need new event listeners for those as well, while you don't want to keep the old ones around since they would be listening to non-existing elements. That's the problem with constructing the DOM again and again based on some state, and why libraries like React or Vue are so popular because they make this very trivial, while with vanilla JS requires more consideration. In fact, in this case, you'd still leak some memory even if you do listen for the click event on the options buttons since you'll only ever click one of them, leaving two other listeners still active. What I'd recommend is having separate functions to recreate those elements so that you can handle event listeners and whatnot separately, without having to keep track of which, when or where you already have. For example:

// Event listener on <main> that pays attention to buttons clicked within it. This is essentially the "game loop"
main.addEventListener('click', (e) => {
  if (e.target.tagName !== 'BUTTON') {
    return;
  }

  // preemptively check for current state of the game to determine what to show

  findWinner(e.target.id);
  updateScore(winner);
  updateScreen(winner);
});

// Creates the options layout from scratch but does not add event listeners
function createOptions() {
  const userOptions = document.createElement('div');
  userOptions.classList.add('userOptions');

  options.forEach(option => {
    const btn = document.createElement('button');
    btn.classList.add('option');
    btn.id = option.type;

    // Do you relly need this div in between?
    const icon = document.createElement('div');
    icon.classList.add('icon');

    const img = document.createElement('img');
    img.src = option.img;
    img.setAttribute('draggable', 'false');

    icon.appendChild(img);
    btn.appendChild(icon);
    userOptions.appendChild(btn);
  });

  main.appendChild(userOptions);
}

// Receives the user input and returns the result of the round.
function findWinner(playerMove) {
  // calculate computer move somehow and who wins
}

function updateRound(winner) {
  // increase/decrease score
}

function updateScreen(winner) {
  // update screen if needed
}

// Event listener on <main> that pays attention to buttons clicked within it. This is essentially the "game loop"
main.addEventListener('click', (e) => {
  if (e.target.tagName !== 'BUTTON') {
    return;
  }

  // preemptively check for current state of the game to determine what to show

  findWinner(e.target.id);
  updateScore(winner);
  updateScreen(winner);
});

// Creates the options layout from scratch but does not add event listeners
function createOptions() {
  const userOptions = document.createElement('div');
  userOptions.classList.add('userOptions');

  options.forEach(option => {
    const btn = document.createElement('button');
    btn.classList.add('option');
    btn.id = option.type;

    // Do you relly need this div in between?
    const icon = document.createElement('div');
    icon.classList.add('icon');

    const img = document.createElement('img');
    img.src = option.img;
    img.setAttribute('draggable', 'false');

    icon.appendChild(img);
    btn.appendChild(icon);
    userOptions.appendChild(btn);
  });

  main.appendChild(userOptions);
}

// Receives the user input and returns the result of the round.
function findWinner(playerMove) {
  // calculate computer move somehow and who wins
}

function updateRound(winner) {
  // increase/decrease score
}

function updateScreen(winner) {
  // update screen if needed
}

MattOP•14mo ago

Thank you for this feedback and thanks for your recommendation I'll working on developing a more optimized solution. I really appreciate the context as it makes practical sense of when to do something / when not to do

Joao•14mo ago

No problem, if you want I can share one attempt that I made myself... it's not perfect of course but perhaps can help to see how others have approach the same problem.

MattOP•14mo ago

Sure! Would be interesting to read. I’ve been pretty busy today, so I haven’t been able to rework a solution. I also saw someone writing their code on YouTube, where they specify the layout on their html markup and hide / show accordingly, instead of adding / removing from DOM

Joao•14mo ago

Yeah there are many ways to do this one, as usual. I find that I'm always gravitating towards the declarative approach similar to how frameworks do it, I guess that's just how I'm used to seeing things. Keep in mind this was also a while back, I actually just corrected a couple of errors on it... 😅 https://codepen.io/D10f/pen/JjpzvMx

Gaming

Programming

Rock, Paper, Scissors (Partial Solution)