High card#63
Conversation
For this, we might want to jump on a call. That way I can fully understand what you mean here as you walk me through the code and behaviour. |
There was a problem hiding this comment.
Seems like you got all images twice in this repo. Once in this folder, once in the src/PNG folder.
| @@ -0,0 +1,184 @@ | |||
| import React from "react"; | |||
There was a problem hiding this comment.
this App copy file makes it confusing for me to review. Do I review the copy or the original? I will review the original only. Going forward I suggest removing such copy files before submitting code for review :)!
| <div key={`${name}${suit}`}> | ||
| {name} of {suit} | ||
| </div> | ||
| if (this.state.currCards === null) { |
There was a problem hiding this comment.
Since the default state value for currCards is an empty array, I wouldn't assume that null is a possible value. If you reset the value, I would always use an empty array for that instead of null for example.
| temp = 0; | ||
| } else if (this.state.currCards[1].rank > this.state.currCards[0].rank) { | ||
| temp = 1; | ||
| } else { | ||
| temp = 99; | ||
| } | ||
|
|
||
| //trigger function to count the total score | ||
| this.setState( | ||
| { | ||
| indexWinner: temp, | ||
| }, | ||
| () => { | ||
| this.countScore(); | ||
| } | ||
| ); | ||
| }; | ||
|
|
||
| countScore = () => { | ||
| //counting score for each player | ||
| const tempCounter = [0, 0]; | ||
| //if indexWinner is 0, add +1 to count index 0, vice versa | ||
| if (this.state.indexWinner === 0) { | ||
| tempCounter[0]++; | ||
| } else if (this.state.indexWinner === 1) { | ||
| tempCounter[1]++; | ||
| } | ||
| //update all the scores | ||
| this.setState((prevState) => ({ | ||
| player1Score: prevState.player1Score + tempCounter[0], | ||
| player2Score: prevState.player2Score + tempCounter[1], | ||
| overallPlayer1Score: prevState.overallPlayer1Score + tempCounter[0], | ||
| overallPlayer2Score: prevState.overallPlayer2Score + tempCounter[1], | ||
| })); |
There was a problem hiding this comment.
This whole section seems a bit overengineered to me :)
You probably could do a simple:
| temp = 0; | |
| } else if (this.state.currCards[1].rank > this.state.currCards[0].rank) { | |
| temp = 1; | |
| } else { | |
| temp = 99; | |
| } | |
| //trigger function to count the total score | |
| this.setState( | |
| { | |
| indexWinner: temp, | |
| }, | |
| () => { | |
| this.countScore(); | |
| } | |
| ); | |
| }; | |
| countScore = () => { | |
| //counting score for each player | |
| const tempCounter = [0, 0]; | |
| //if indexWinner is 0, add +1 to count index 0, vice versa | |
| if (this.state.indexWinner === 0) { | |
| tempCounter[0]++; | |
| } else if (this.state.indexWinner === 1) { | |
| tempCounter[1]++; | |
| } | |
| //update all the scores | |
| this.setState((prevState) => ({ | |
| player1Score: prevState.player1Score + tempCounter[0], | |
| player2Score: prevState.player2Score + tempCounter[1], | |
| overallPlayer1Score: prevState.overallPlayer1Score + tempCounter[0], | |
| overallPlayer2Score: prevState.overallPlayer2Score + tempCounter[1], | |
| })); | |
| else if (this.state.currCards[0].rank > this.state.currCards[1].rank) { | |
| this.setState((prevState) => ({ | |
| player1Score: prevState.player1Score + 1, | |
| overallPlayer1Score: prevState.overallPlayer1Score + 1, | |
| })); | |
| } else if (this.state.currCards[1].rank > this.state.currCards[0].rank) { | |
| this.setState((prevState) => ({ | |
| player2Score: prevState.player2Score + 1, | |
| overallPlayer2Score: prevState.overallPlayer2Score + 1, | |
| })); | |
| } else { | |
| <do some tie thing> | |
| } |
| <img | ||
| src={require(`./PNG/${name.toLowerCase()}_of_${suit.toLowerCase()}.png`)} | ||
| alt={`${name} of ${suit}`} | ||
| style={{ height: "200px" }} | ||
| /> |
There was a problem hiding this comment.
You could potentially make this an image component with name, suit, height and width props.
| this.state.indexWinner === -1 | ||
| ? "" | ||
| : this.state.indexWinner === 99 | ||
| ? `Tie` | ||
| : `Player ${this.state.indexWinner + 1} won!`; |
There was a problem hiding this comment.
Please don't use nested ternary operators. This is very hard to read haha :D
I suggest multiple if statements, a function or possible a switch statement with the last else as default value.
| <p>Overall 🏆{this.state.overallPlayer2Score}</p> | ||
| </div> | ||
| </div> | ||
| {console.log("Player 1 Score:", this.state.player1Score)}; |
There was a problem hiding this comment.
Always clean up console logs in your code before submitting for review or pushing code to production
| </div> | ||
|
|
||
| <h2 className="text-center mt-2"> | ||
| {numRoundsLeft > 0 && winnerMessage} |
There was a problem hiding this comment.
If winnermessage is the empty string as above, you render the h2 unnecessarily. I would do it like so:
| {numRoundsLeft > 0 && winnerMessage} | |
| {numRoundsLeft > 0 && winnerMessage && | |
| (<h2 className="text-center mt-2">{winnerMessage}</h2>) | |
| } |
| </h2> | ||
|
|
||
| <h1 className="text-center mt-2"> | ||
| {numRoundsLeft === 0 && gameWinnerMessage} |
There was a problem hiding this comment.
Same here. Plus we can improve the variable naming here. What is the difference between winner and gameWinner? I mean it can be clear, but also not
2 participants
The commit about "updated bug - the winner was calculated using round 25 instead of round 26", I am still not 100% clear why it was behaving that way even though I managed to fix them.
I tried to console log
this.state.player1Scorein thecheckWinner()and it is not showing the latestplayer1Scorebut it is showing the previous roundplayer1Score. From my understanding,this.state.player1Scoreshould have shown the latest updated number as whenever card is dealt it will trigger an update to the playerScore.