3 min read

Code review: reader solution to Memory Game

This is a code review of the solution to the Memory game exercise that a user submitted.
Repository: https://github.com/stivendiaz/memory-game
Page: https://stivendiaz.github.io/memory-game/

Thoughts at first glance:

  • great that the app is responsive and uses Typescript
  • really cool that is also deployed to Github Pages

Now let's go over the implementation step by step:

Initialising the state

  • ✅  Good example of using useEffect to keep the initial value updated when the props change; in this scenario it's an appropriate use, as gameImages gets changed further down the line; if we didn't need to change it, then a simple const gameImages = createGameImages(images) would have been enough. Read more here and here;

The select event handler

  • ⚠️  When going over the comparison img.index === image.index, it's hard to see immediately which image is which - maybe consider using more clear names, for example selectedImage instead of just image?
  • ⚠️ You should avoid using useCallback unless there is a clear performance optimisation you're working towards. See the docs for a detailed description.

The effect that checks the status of the round

  • ❌ Updating state with a timeout should not be neccessary - it's usually a red flag that you're using an anti-pattern somewhere else. I expect that by moving the code above to the handleSelect event handler, you'll no longer need this!

Here is an example of an updated handleSelect, that includes the code that checks the round status and no longer has the set timeout:

const handleSelect = (image: ImageCard) => {
    const selectedCurrentRound = [...selectedInRound, image.index]
    const updatedImages = gameImages.map((img) =>
        img.index === image.index ? { ...img, isSelected: true } : img
    // if round is over - i.e. user clicked two images
	if (selectedCurrentRound.length === 2) {
      const [first, second] = selectedCurrentRound;
      // if we don't have a match
      if (
        gameImages[first].src !== gameImages[second].src ||
        first === second
      ) {
	      // flip images back around
			updatedImages.map((img) =>
			  img.index === first || img.index === second
				? { ...img, isSelected: false }
				: img
      // and reset the round (aka the selected indexes)

	  // if at the end of the round all images are selected
	  // reset the game
      const isGameFinished = updatedImages.every((img) => img.isSelected);
      if (isGameFinished) setGameImages(createGameImages(images));

    } else {
    // else continue the game

Other notes

  • ✅ Very nice clean way to structure the code - clear separation of concerns, each function does one thing (i.e. shuffleArray is its own function); Tip: consider using lodash's library shuffle method instead of custom implementation
  • ✅ Clear naming of variables, immediately clear what each one holds
  • ✅ Another plus one for extracting the isSelected check into a variable, to improve readability
  • ⚠️ Consider️ renaming the img prop to simply image - e.g. <Card image={img} .../>
  • ⚠️ Consider️ renaming handleSelect prop to onSelect - e.g. `<Card onSelect={handleSelect} .../>