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:
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-pTgwgvgH11 Replies
Blue = Paper
Yellow = Scissors
Red = Rock
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.Thanks for your feedback
Adding this as third parameter doesn't fix the issue with listening for reset click
My current problem with this, is that when added outside of main function, it results in error beause #reset is non-existent
The options should be the 3rd parameter of
addEventListener
so it would look something like this:
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.
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:
as once the buttons are removed from DOM and re-added, they need the eventlistener again
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:
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
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.
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
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