Skip to content

27-3 Jia Hui#576

Open
sinsinex wants to merge 6 commits intorocketacademy:mainfrom
sinsinex:main
Open

27-3 Jia Hui#576
sinsinex wants to merge 6 commits intorocketacademy:mainfrom
sinsinex:main

Conversation

@sinsinex
Copy link

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.

Copy link

@floatingtales floatingtales left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job Jia Hui! This is a very neat beat that project! I'm excited to see what you can do for the blackjack project!

.hidden-text {
display: none;
}
</style>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice way of making this a one-liner!

var player2WinCount = 0;
var player1LoseCount = 0;
var player2LoseCount = 0;
var drawCount = 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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") {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants