Memory Game in React

Question

This is my second game in React (I posted a country guessing game 2 days ago). Please review my code.

Preview

Alert: May work only in Google Chrome (I build here)

import React, { Component } from 'react';
import './MemoryGame.css';
class MemoryGame extends Component {
 constructor(props) {
 super(props);
 //Array of memory images
 this.ImagePieces = ['cat', 'cat', 'dog', 'dog', 'horse', 'horse',
 'pig', 'pig', 'snake', 'snake', 'fish', 'fish'];
 this.tempCheckArr = [];
 this.state = {
 showImg: Array(this.ImagePieces.length).fill('hidden'),
 divClick: true,
 compareImgArr: [],
 counter: 0
 } 
 this.checkMatch = this.checkMatch.bind(this);
 }
 //Shuffle memory game images
 componentWillMount() {
 function shuffleArray(array) {
 for (let i = array.length - 1; i > 0; i--) {
 let j = Math.floor(Math.random() * (i + 1));
 [array[i], array[j]] = [array[j], array[i]];
 }
 return array;
 }
 shuffleArray(this.ImagePieces);
 }
 //Check for match function
 checkMatch(key, e) {
 //For later hidding images purposes
 this.tempCheckArr.push(key.toString());
 //Create copy of (compareImgArr) and add img src, for later compare
 const imgSrc = e.target.firstChild.src;
 const compareImgArr = [...this.state.compareImgArr];
 compareImgArr.push(imgSrc);
 //Set current clicked item as 'visible' in main array 'showImg'
 const arr = this.state.showImg
 arr[key] = 'visible';
 //Update state, counter for block user click method
 //after unhidding two pieces
 this.setState({
 showImg: arr,
 compareImgArr: compareImgArr,
 counter: this.state.counter + 1
 });
 //Check if 2 items are clicked - if yes - disable clicking
 if (this.state.counter % 2) {
 this.setState({
 divClick: false
 });
 //Check if pictures are matching
 if (compareImgArr[0] === compareImgArr[1]) {
 this.tempCheckArr = [];
 this.setState({
 compareImgArr: [],
 divClick: true
 });
 } else {
 //If pictures not match turn them back to hidden
 var tempArr = this.state.showImg
 // eslint-disable-next-line
 var firstElement = parseInt(this.tempCheckArr[0]);
 // eslint-disable-next-line
 var secondElement = parseInt(this.tempCheckArr[1]);
 setTimeout(()=>{
 tempArr[firstElement] = 'hidden';
 tempArr[secondElement] = 'hidden';
 this.tempCheckArr = [];
 this.setState({
 showImg: tempArr,
 compareImgArr: [],
 divClick: true
 })
 }, 1500)
 }
 }
 }
 render() {
 return(
 <div>
 <h1>Memory Game</h1>
 <div className="mui-panel wrapper">
 {this.ImagePieces.map((text, i) => {
 return (
 <div key={i} className="modal mui-panel" 
 onClick={this.state.divClick ? (e) => this.checkMatch(i, e) : undefined}>
 <img style={{visibility: this.state.showImg[i]}} src={'./'+text+'.jpg'}
 srcSet={'./'+text+'_lrg.jpg 1000w'} key={i} alt="Game Element"/>
 </div>
 )
 })}
 </div>
 </div>
 )
 }
}
export default MemoryGame;

Answer 1

Ok, I had quite a few thoughts on this.

State

Your state seems more complex than it needs to be, and some of it is hiding outside of the state variable. I think all you really need to keep track of are: - What order the images are in - Which images are currently selected - Which images have been guessed correctly

I don't really understand what tempCheckArr is, how it's different from compareImgArr, or why it's an instance variable.

Extract stuff

I think you have too much stuff crammed into one component. I think you need to extract pieces of logic that can be self contained.

Prime example: shuffleArray

Why is shuffleArray declared inside componentWillMount? This is creating a new shuffleArray function every time MemoryGame is mounted. I think shuffleArray should at least be outside of this component, and really should be in a some sort of RandomUtil.js file.

Card component

I would also extract out a Card component that renders a card. It should know: - What image to display - Whether or not it's selected - Whether or not it's been guessed correctly

This way you can keep your MemoryGame component about managing the state of the game while the Card component focuses on presentation.

Other Thoughts

• Your <img> tag doesn't need a key prop because it's not the top level thing in an array.
• Why is ImagePieces declared on this. It seems like it should be part of state.
• (Also why is the I in ImagePieces uppercase? It seems like this should be imagePieces)
• Why are you shuffling ImagePieces in componentWillMount and not just in the constructor?
• It's kinda gross to reach into the DOM to compare the elements src attribute, and it makes it harder to understand what you actually want to compare: that the two images selected are of the same thing
• Why are you converting integers to strings when you put them into tempCheckArr and converting them back when you take them out? Wouldn't it be easier to just put integers into the array?

How I would write it

const IMAGES = ["cat", "cat", "dog", "dog", "horse", "horse", "pig",
 "pig", "snake", "snake", "fish", "fish"];
class MemoryGame extends Component {
 constructor(props) {
 super(props);
 // You can simplify your state a lot
 this.state = {
 cards: shuffleArray(IMAGES.slice()),
 selected: [], // indexes which have been selected
 correct: [] // indexes which have been guessed correctly
 };
 }
 // Don't need a componentWillMount
 onCardClick(clickedIndex) {
 const { selected, cards, correct } = this.state;
 if (selected.length === 0) { // selecting a first card
 this.setState({ selected: [clickedIndex] })
 } else if (selected.length === 1) { // they're selecting a second card
 if (cards[selected[0]] === cards[clickedIndex]) {
 // It's a match :)
 // Add selected cards to `correct` and reset selection
 this.setState({
 correct: correct.concat([selected[0], clickedIndex]),
 selected: []
 });
 } else {
 // It's not a match :(
 // Select it for now, and reset selection in a bit
 this.setState({ selected: [selected[0], clickedIndex] });
 setTimeout(() => {
 this.setState({ selected: [] })
 }, 1500);
 }
 }
 // Otherwise they already have 2 selected and we don't wanna do anything
 }
 render() {
 const { correct, selected, cards } = this.state;
 return (
 <div>
 <h1>Memory Game</h1>
 <div className="mui-panel wrapper">
 {cards.map((image, i) => (
 <MemoryCard
 key={i}
 image={image}
 isCorrect={correct.includes(i)}
 isSelected={selected.includes(i)}
 onSelect={() => this.onCardClick(i)}
 />
 ))}
 </div>
 </div>
 );
 }
}
// Extracted into it's own component
const MemoryCard = ({ image, isSelected, isCorrect, onSelect }) => (
 <div
 className="modal mui-panel"
 onClick={() => {
 // You can only select a card that's not already correct and
 // isn't currently selected
 if (!isCorrect && !isSelected) {
 onSelect();
 }
 }}
 >
 <img
 style={{ visibility: (isCorrect || isSelected) ? 'visible' : 'hidden' }}
 src={"./" + image + ".jpg"}
 srcSet={"./" + image + "_lrg.jpg 1000w"}
 alt={image}
 />
 </div>
);
// Probably in a different file
function shuffleArray(array) {
 for (let i = array.length - 1; i > 0; i--) {
 let j = Math.floor(Math.random() * (i + 1));
 [array[i], array[j]] = [array[j], array[i]];
 }
 return array;
}
export default MemoryGame;