Skip to content
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

submission #142

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

submission #142

wants to merge 1 commit into from

Conversation

arnolddhu
Copy link

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

Completeness Level (1-5):

What did you think of this deliverable?

Is there anything in this code that you feel pleased about?

@codeclimate
Copy link

codeclimate bot commented Aug 3, 2021

Code Climate has analyzed commit 8d4bf6b and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 2

View more on Code Climate.

Copy link

@eddiejpot eddiejpot left a comment

Choose a reason for hiding this comment

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

Good job! looking forward to the final project !

Comment on lines +1 to +4
// 2 different game modes

var firstGameMode = "Game Mode Dice Roll";
var secondGameMode = "Game Mode Choose Order";

Choose a reason for hiding this comment

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

Good use of declaring gameModes as variables.

We could use scream_case for the naming so we won't confuse them with the other variables.

E.g.
FIRST_GAME_MODE = "Game Mode Dice Roll"

Comment on lines +19 to +27
var getDiceRolls = function () {
var newDiceRolls = [getDiceRoll(), getDiceRoll()];
if (player == 1) {
playerOneDiceRoll = newDiceRolls;
} else {
playerTwoDiceRoll = newDiceRolls;
}
return newDiceRolls;
};

Choose a reason for hiding this comment

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

Same thing as what we learnt in class about how we try to avoid assigning arrays like this. It's better to do the push method

Reason being this idea of reference vs primitive types.
Watch this video to find out more :)

Comment on lines +29 to +31
var concatenate2Numbers = function (num1, num2) {
return Number(String(num1) + String(num2));
};

Choose a reason for hiding this comment

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

Good helper function and naming :)

Comment on lines +55 to 85
if (currentGameMode == firstGameMode) {
var newDiceRolls = getDiceRolls();
currentGameMode = secondGameMode;
return `Welcome Player ${[player]}<br>
You rolled Dice 1: ${newDiceRolls[0]} and Dice 2: ${newDiceRolls[1]} <br>
Choose the order of the dice by entering 1 or 2 as the first numeral index.`;
}
if (currentGameMode == secondGameMode) {
var firstNumeralIndex = Number(input);
if (firstNumeralIndex !== 1 && firstNumeralIndex !== 2) {
return `Please choose 1 or 2 as your first numeral index for your dice rolls!`;
}
var playerNum = getPlayerNumber(firstNumeralIndex);
var playerNumResponse = `Player ${player}, You chose Dice ${firstNumeralIndex} first. <br>
Your number is ${playerNum}.`;
}
if (player == 1) {
player = 2;
currentGameMode = firstGameMode;
return `${playerNumResponse} <br>
It is now Player 2's turn. Press Submit to roll dice.`;
}
var winningPlayer = determineWinner();
//Reset the game
player = 1;
currentGameMode = firstGameMode;
return `${playerNumResponse} <br>
Player ${winningPlayer} has won! <br>
Player 1's number: ${playerOneNum} & Player 2's number: ${playerTwoNum}<br>
Press submit to play again!`;
};

Choose a reason for hiding this comment

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

Nice work using helper functions to handle the 'heavy lifting' and just use the main function to return sentences!

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