Skip to content

BC High Card - Hong Yun#62

Open
magiicloud wants to merge 17 commits intorocketacademy:mainfrom
magiicloud:main
Open

BC High Card - Hong Yun#62
magiicloud wants to merge 17 commits intorocketacademy:mainfrom
magiicloud:main

Conversation

@magiicloud
Copy link

No description provided.

currComputerCards: newComputerCurrCards,
roundsLeft: this.state.roundsLeft - 1,
},
() => this.determineWinner()

Choose a reason for hiding this comment

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

Suggested change
() => this.determineWinner()
this.determineWinner

I think you should be able to just pass the function itself like so

dealCards = () => {
// this.state.cardDeck.pop() modifies this.state.cardDeck array
const newCurrCards = [this.state.cardDeck.pop(), this.state.cardDeck.pop()];
const newCurrCards = [this.state.cardDeck.pop()];

Choose a reason for hiding this comment

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

Since it is just a single card, do we need an array? Also, does the variable need to be in plural?

() => this.determineWinner()
);
if (this.state.cardDeck.length === 0) {
this.setState(() => this.determineOverallWinner());

Choose a reason for hiding this comment

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

determineOverallWinner() is already updating state. I think we don't need to call the function within a state update

};

determineWinner = () => {
const playerRank = this.state.currCards[0].rank;

Choose a reason for hiding this comment

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

could currCards be empty? If so, you could have an edge case bug here, trying to access .rank on undefined

Comment on lines +98 to +115
<>
<div key={`${name}${suit}`}>{/*{name} of {suit}*/}</div>
<div>
<PlayingCard value={name} suit={suit} />
</div>
</>
));

const currComputerCardElems = this.state.currComputerCards.map(
({ name, suit }) => (
// Give each list element a unique key
<>
<div key={`${name}${suit}`}>{/* {name} of {suit}*/}</div>
<div>
<PlayingCard value={name} suit={suit} />
</div>
</>
)
);

Choose a reason for hiding this comment

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

We repeat the same html here. Could this be a separate component, into which we pass the name and suit prop?

<p>Computer Hand {currComputerCardElems}</p>
</div>
)}
{this.state.roundsLeft !== 26 && (

Choose a reason for hiding this comment

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

what is the 26? It seems to be some kind of game logic, but is not explained here. Maybe it would be better to pack it into a variable, say maxNumberOfRounds = 26 or whatever it represents. This would make it easier to read the code here as the variable would explain in English what it is used for or means.

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