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;