-
Notifications
You must be signed in to change notification settings - Fork 660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basics-Beat-That-Submission: Ying Ling #575
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great attempt at beat that, Ying Ling! Can't wait to see what you can do for blackjack!
while (counter < 2) { | ||
currentPlayerRolls.push(rollDice()); | ||
counter = counter + 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this instance it might be better to use a for loop as opposed to a while loop. You already know how many times the loop will run!
var GAME_STATE_DICE_ROLL = "GAME_STATE_DICE_ROLL"; | ||
var GAME_STATE_CHOOSE_DICE_ORDER = "GAME_STATE_CHOOSE_DICE_ORDER"; | ||
var GAME_STATE_COMPARE_SCORES = "GAME_STATE_COMPARE_SCORES"; | ||
var gameState = GAME_STATE_DICE_ROLL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great usage of constants here!
|
||
// Helper Function | ||
var rollDice = function () { | ||
console.log("Control flow: start of rollDice()"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console logs usually are internal checks for your use only, it usually is omitted for final code submissions.
// tie | ||
if (allPlayersScore[0] == allPlayersScore[1]) { | ||
compareMessage = compareMessage + "<br><br>It's a tie!"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slight nitpick here, it might be better to use:
if(...){
// logic
} else if(...){
// logic
} else {
// logic
}
if/else if/else is better than consecutive ifs here.
Cool! thanks for the feedback @floatingtales |
Please fill out the survey before submitting the pull request. Thanks!
🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀
How many hours did you spend on this assignment?
Please fill in one error and/or error message you received while working on this assignment.
What part of the assignment did you spend the most time on?
Comfort Level (1-5): 2 (because of the walk through's help)
Completeness Level (1-5): 5
What did you think of this deliverable? (I couldn't have done it w/o the walk through)
Is there anything in this code that you feel pleased about? (I finished the walk through, even though I got lost a few times throughout).
What's one aspect of your code you would like specific, elaborate feedback on? Nothing. But I wish the basics could be recap again.