27-3 Jia Hui #576
27-3 Jia Hui #576
Conversation
| .hidden-text { | ||
| display: none; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
great that you've added style tags!
But this belong within the <header> header </header> as opposed to the <body> body </body>
| <p class="instruction-text"> | ||
| Ready to play? Player 1, click submit to start the game. | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
Great that you've added instructions here! Also great usage of classes and ids here!
Slight nitpick, id names usually have the same case styling as class names, they're in what we call kebab-case. So it might be better to name the id tag as game-instructions
| var instructions = document.querySelectorAll(".instruction-text"); | ||
| instructions.forEach(function (instruction) { | ||
| instruction.classList.add("hidden-text"); | ||
| }); |
There was a problem hiding this comment.
Awesome job on doing this! You do know how to take in a tag with the querySelector and hide it! I don't particularly like the array.forEach() function because it's a bit finicky, but this works!
p.s. I'd like to use a for loop to iterate through this
for(var i = 0; i<instructions.length; i+=1){
instructions[i].classList.add("hidden-text")
}
| var gameMode = modeRollDice; | ||
|
|
||
| var rollSingleDice = function () { | ||
| return Math.floor(Math.random() * 6) + 1; |
There was a problem hiding this comment.
nice way of making this a one-liner!
| var player2WinCount = 0; | ||
| var player1LoseCount = 0; | ||
| var player2LoseCount = 0; | ||
| var drawCount = 0; |
There was a problem hiding this comment.
great usage of global variables here!
But there are slightly redundant variables here. because player1WinCount will always be exactly the same as player2LoseCount, and vice versa.
| }; | ||
|
|
||
| var rollDiceAndAssign = function () { | ||
| var getDiceNumber = [rollSingleDice(), rollSingleDice()]; |
There was a problem hiding this comment.
This works for a 2 dice game. It might be a bit challenging if you want to expand this into having variable amount of dice tho!
| var rollDiceAndAssign = function () { | ||
| var getDiceNumber = [rollSingleDice(), rollSingleDice()]; | ||
|
|
||
| if (currentPlayer === "Player 1") { |
There was a problem hiding this comment.
ahh yes! A === strict equality operator!
It actually is a more recommended way of doing as opposed to a == equality operator.
the only difference being the === checks the data type as well as the value. as opposed to a ==.
i.e.
1=="1" //true
1==="1" //false
|
|
||
| //Combine numbers based on chosen order and show user combined number | ||
| if (gameMode === modeChooseDiceOrder) { | ||
| var orderChosen = Number(input); |
There was a problem hiding this comment.
this is why you need to cast the string into a number, because of the strict equality!
However the way that you chose to do it might have a weird edge case that should be considered:
try inputting a Hex-value string like say 0x000001
It will still run!
Please fill out the survey before submitting the pull request. Thanks!
🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀
How many hours did you spend on this assignment? At least 8 hours.
Please fill in one error and/or error message you received while working on this assignment. Unable to commit properly to git, functions not working the way I intended
What part of the assignment did you spend the most time on? Trying to understand how I can get the game to run for a second time for the second player.
Comfort Level (1-5): 3 (assuming 1 being least comfortable - this was rather challenging for me)
Completeness Level (1-5): 3 (I hit the basics and I think that's good enough for me)
What did you think of this deliverable? Definitely a step up from the scissors-paper-stone assignment, but still doable.
Is there anything in this code that you feel pleased about? Figuring out how to combine the two numbers based on the user's input.
What's one aspect of your code you would like specific, elaborate feedback on? If there's better ways to refactor the code.