Skip to content

At long last 😀#60

Open
ianthehamster wants to merge 6 commits into
rocketacademy:mainfrom
ianthehamster:main
Open

At long last 😀#60
ianthehamster wants to merge 6 commits into
rocketacademy:mainfrom
ianthehamster:main

Conversation

@ianthehamster
Copy link
Copy Markdown

No description provided.

Comment thread src/AppOriginal.js
dealCards = () => {
if (this.state.cardDeck.length < 2) {
// Check if there are enough cards in the deck
return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this check is good, but you are not handling it however. A simple return would leave the user with a blank screen. Maybe do something else, like making a new deck, rendering some text inform the user about what happens etc.

Comment thread src/AppOriginal.js
winner: null, // Resets the winner when dealing new cards
});

console.log(this.newCurrCards);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Always remove console.logs before pushing code

Comment thread src/AppOriginal.js
Comment on lines +69 to +70
this.setState((prevState) => ({
player1Score: prevState.player1Score + 1,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think you could just add the winner state update into the respective player's updates.

Suggested change
this.setState((prevState) => ({
player1Score: prevState.player1Score + 1,
this.setState((prevState) => ({
player1Score: prevState.player1Score + 1,
winner: this.player1

Comment thread src/AppOriginal.js
/////////////
// Extract cards and determine winner
// extracts the two cards and saves the 2 objects into the class properties, player1 and player2
[this.player1, this.player2] = this.newCurrCards;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does this work? All I see this that you define the variables as this.player1. It is not exactly a this.player1 = this.newCurrCards[0].

Why do you not store the players in state by the way? Especially since these values get updated and need to be persistent values?

Comment thread src/AppOriginal.js
The overall winner is:{" "}
{this.state.player1Score > this.state.player2Score
? "Player 1"
: "Player 2"}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

So for a tie, Player 2 would be the overall winner?

Comment thread src/AppOriginal.js
@@ -0,0 +1,171 @@
import React from "react";
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I am a bit confused. What is the difference between App.js and AppOriginal.js? I only just noticed those are 2 different files, which makes my review a bit unreliable. Please only keep 1 copy of a component in your repositories, as this would confuse anyone reviewing or reading your code

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