4
\$\begingroup\$

I have attempted to create a hangman game in JavaScript. Some parts of my code is a bit DRY. Basically, the Hangman class is a wrapper for all of the available actions in a typical hangman game: setup the board, generate a word, detect game end, etc. This class will be integrated into my UI logic part of the project. I know that the code can be improved. Please note that this code does not include the UI logics, just pure game logic.

// The hangman class will handle all of the hangman-related logics.
export class Hangman extends EventTarget {
    constructor() {
      super();
    }
  
    async setup() {
      // Defines some important hangman game properties.
      try {
        this.allWords = await this.fetchWords();
      } catch(error) {
        throw error;
      }
      this.gameState = null;
      this.word = this.allWords[0];
      this.attempts = this.word.length + 5;
      // Get the number of underscores needed for the game.
      this.gameStateWordString = [...this.word].map(() => '_').join('');
    }
  
    async fetchWords() {
      // This method fetches around 2000 words.
      let data;
      try {
        data = await fetch("https://random-word-api.herokuapp.com/word?number=2000");
      } catch (error) {
        throw error;
      }
      return await data.json();
    }
  
    removePlayedWord() {
      this.allWords.shift();
    }
  
    // Resets everything and pick a new word. It also checks if the word amount is low.
  // Yeah, this method is a bit DRY.
    async playAnotherRound() {
      this.removePlayedWord();
      if (this.allWords.length === 0) {
        try {
          this.allWords = await this.fetchWords();
        } catch (error) {
          throw error;
        }
      }
      this.gameState = null;
      this.word = this.allWords[0];
      this.attempts = this.word.length + 5;
      this.gameStateWordString = [...this.word].map(() => '_').join('');
    }
  
    insertLetter(letter) {
      const currentWordStateArray = [...this.gameStateWordString];
      const letterExists = this.word.includes(letter);
      if (!letterExists) {
        this.decreaseAttempts();
        this.detectGameEnd();
        return;
      }
      const letterPositions = this.findAllOccurancesOfLetterIndex(letter);
      letterPositions.forEach((index) => {
        currentWordStateArray[index] = letter;
      });
      this.gameStateWordString = currentWordStateArray.join('');
      this.decreaseAttempts();
      this.detectGameEnd();
    }
  
    // Finds all the position of a certain letter in a word.
    findAllOccurancesOfLetterIndex(letter) {
      const currentWord = this.word;
      return [...currentWord].map((currentLetter, index) => {
        if (letter === currentLetter) return index;
      });
    }
  
    decreaseAttempts() {
      this.attempts -= 1;
    }
  
    dispatchGameEndEvent() {
      const event = new CustomEvent("gameend");
      this.dispatchEvent(event);
    }
  
    detectGameEnd() {
      if ((this.gameStateWordString === this.word && this.attempts === 0) || this.gameStateWordString === this.word) {
        // Ensures that the "gameend" event will only be fired once.
        if (this.gameState) return;
        this.gameState = "win";
        this.dispatchGameEndEvent();
      } else if (this.attempts === 0 && this.gameStateWordString !== this.word) {
        this.gameState = "lose";
        this.dispatchGameEndEvent();
      }
    }
  }

This class is just a game logic class. It does not include any DOM manipulation logics. I wanted to proceed with my project after the community has reviewed my code, so that it will be much easier for me to add new functionalities to the class.

\$\endgroup\$
10
  • \$\begingroup\$ @ggorlen, I wanted to ensure that this code is of good quality before implementing the UI part of this project. \$\endgroup\$ Apr 13 at 18:46
  • \$\begingroup\$ Ah, I see there is no DOM manipulation here so that's fine. Just curious why you're using events here, though, just for the class to communicate with itself? \$\endgroup\$
    – ggorlen
    Apr 13 at 20:01
  • \$\begingroup\$ @ggorlen, I have also written the UI part of this project in JS. Esssentially, I am going to add an event listener to the hangman object, which is instantiated from the Hangman class. I wanted to communicate the hangman UI to this class. \$\endgroup\$ Apr 13 at 20:19
  • \$\begingroup\$ It's still not clear to me why you'd use events rather than simple function calls. \$\endgroup\$
    – ggorlen
    Apr 13 at 20:22
  • \$\begingroup\$ @ggorlen, Why not? It just makes the code easier to understand and more intuitive to look at. For example, const hangman = new Hangman(); hangman.addEventListener("gameend", handleGameEnd); - my mind will automatically associate the last line as a handler for when the game has ended. \$\endgroup\$ Apr 13 at 21:08

1 Answer 1

3
\$\begingroup\$

Avoid pointless comments

// Fetch words for the game
this.allWords = await this.fetchWords();

The comment repeats what the code clearly does; remove it.

Use standard doc-style comments

Your comments would benefit from following a standard TSDoc or JSDoc style:

/**
 * Fetches n random words from a word API.
 * @param {number} [n=100] - The number of words to pull.
 * @returns {Promise<string[]>} A promise resolving to an array of words.
 * @throws {Error} If word fetching fails.
 */
async #fetchWords(n = 100) {
  // ...
}

Comments like this can be used to automatically generate documentation and facilitate type checking. Nowadays, GPTs are pretty good for quickly generating these sort of comments so there's no excuse.

Comment hygiene

The tone of // Yeah, this method is a bit DRY. is a bit too casual for my taste. The note itself doesn't quite make sense because DRY is usually a good thing. Did you mean WET (wrote everything twice)?

Be clear in comments: // Defines some important hangman game properties. leaves me guessing. If the properties are important, why not list them or describe what makes them important? This comment can probably be removed.

Error handling

There's no point in catching an error just to throw it again:

try {
  this.allWords = await this.fetchWords();
} catch(error) {
  throw error;
}

can be

this.allWords = await this.fetchWords();

ESLint flagged this as error Unnecessary try/catch wrapper no-useless-catch. Always lint your code.

That said, fetch doesn't throw for non-OK status codes. The general pattern is:

if (!response.ok) {
  throw Error(response.statusText);
}

or similar. This avoids a confusing error if you get a non-OK status and try to parse it as JSON.

The resolved value of fetch() isn't the data, it's a Response. The data only exists after await response.json(). Name your variables accordingly.

return await data.json() can be return data.json().

If you don't want to expose the fetch implementation detail to the client, you can catch this error and raise a custom error.

Inconsistent indentation

Your class switches between 2 and 4 spaces. Don't worry about formatting, just use Prettier on everything.

Eliminate duplicate functions

playAnotherRound() and setup() are pretty much identical. Combine them.

Hardcoding

Move URLs and magic strings like 5, "_", "win" and so forth to static class properties. Prefer symbols to magic strings.

Simplify conditions

if ((this.gameStateWordString === this.word && this.attempts === 0) || this.gameStateWordString === this.word) {

is the same as if (this.gameStateWordString === this.word) {}.

Variable names

Your variable names are a bit verbose without being necessarily clearer. I'm not sure what a gameState is exactly, but it probably doesn't need to be used since it's clear that the class represents a hangman game state.

Generally, avoid words like String in variables. Let type annotations clarify types.

Function names should also be short and clear.

Avoid events (unless you really need them)

Your CustomEvent is one too clever for me. I don't see why allowing the caller to provide a simple callback wouldn't achieve the same result with less verbosity.

For that matter, since the client is invoking insertLetter, they can run a check manually--there's no need to bring in anything async like an event emitter. The only async work that needs to be done here is the fetch call.

Single responsibility functions

insertLetter is very overburdened. It does a lot more than just insert a letter--it decreases attempts and triggers events as well. This function should follow the single responsibility principle. It's also a bit WET, with the this.decreaseAttempts(); this.detectGameEnd(); calls happening regardless of which branch is taken. So instead of:

if (!letterExists) {
  this.decreaseAttempts();
  this.detectGameEnd();
  return;
}
const letterPositions = this.findAllOccurancesOfLetterIndex(letter);
letterPositions.forEach((index) => {
  currentWordStateArray[index] = letter;
});
this.gameStateWordString = currentWordStateArray.join('');
this.decreaseAttempts();
this.detectGameEnd();

You can use:

if (letterExists) {
  const letterPositions = this.findAllOccurancesOfLetterIndex(letter);
  letterPositions.forEach((index) => {
    currentWordStateArray[index] = letter;
  });
  this.gameStateWordString = currentWordStateArray.join('');
}

this.decreaseAttempts();
this.detectGameEnd();

If the if () {} block is too long, you can factor it out to a function.

Use simpler logic

[...this.word].map(() => '_').join('');

can be:

"_".repeat(this.word.length);

Confusing return values

findAllOccurancesOfLetterIndex (which has a spelling error) has a rather confusing return value. It returns an array of undefined and numbers, leaving the caller to work out more details. Try to create abstractions that return the sort of structures that don't require much thinking or further processing to use.

Don't shift, pop

Avoid shifting arrays which are O(n); pop is O(1). This is a micro-optimization, but since the array is random, it's the same result either way and you may as well use the "normal" approach, which is better hygiene, reduces code smell and calls less attention to itself.

Use private properties

Private properties and functions help clarify the public interface for the class and make it easier to understand the code.

Add a test harness

How do you know this code works? Add tests that call the code.

Rewrite suggestion

hangman.mjs:

/**
 * Represents a Hangman word guessing game.
 */
export default class Hangman {
  /**
   * The URL of the API used to fetch words.
   * @type {string}
   */
  #apiUrl = "https://random-word-api.herokuapp.com/word";

  /**
   * The character used to represent a blank space in the hidden word.
   * @type {string}
   */
  #blankSpace = "_";

  /**
   * The number of extra attempts allowed in the game.
   * @type {number}
   */
  #extraAttempts = 5;

  /**
   * The array of words to choose from.
   * @type {string[]}
   */
  #words = [];

  /**
   * The number of guesses left.
   * @type {number}
   */
  #guesses = 0;

  /**
   * The current word being guessed.
   * @type {string[]}
   */
  #word = [];

  /**
   * The hidden representation of the word being guessed.
   * @type {string[]}
   */
  #hiddenWord = [];

  /**
   * Constructs a new Hangman instance.
   */
  constructor() {}

  /**
   * Fetches words if necessary and initializes game state.
   * @throws {Error} If fetching fails.
   */
  async newGame() {
    if (this.#words.length === 0) {
      this.#words = await this.#fetchWords();
    }

    this.#word = [...(this.#words.pop() ?? [])];
    this.#guesses = this.#word.length + this.#extraAttempts;
    this.#hiddenWord = [...this.#blankSpace.repeat(this.#word.length)];
  }

  /**
   * Returns the revealed word after the game is over.
   * @returns {string} The revealed word.
   * @throws {Error} If called with an active game.
   */
  get revealedWord() {
    if (this.isActive) {
      throw new Error("Cannot show the hidden word until the game is over");
    }

    return this.#word.join("");
  }

  /**
   * Returns the hidden word. Letters the player has guessed so far
   * are displayed; the rest of the letters are hidden.
   * @returns {string} The partially or fully hidden word.
   */
  get hiddenWord() {
    return this.#hiddenWord.join("");
  }

  /**
   * Returns whether the game is won. A game is considered won if the
   * player has revealed all letters in the word.
   * @returns {boolean} True if the game is won, false otherwise.
   */
  get isWon() {
    return this.#hiddenWord.every((e) => e !== this.#blankSpace);
  }

  /**
   * Returns whether the game is lost. A game is considered lost if the
   * player has exhausted their guesses and there are still unrevealed letters.
   * @returns {boolean} True if the game is lost, false otherwise.
   */
  get isLost() {
    return (
      this.#guesses <= 0 && this.#hiddenWord.some((e) => e === this.#blankSpace)
    );
  }

  /**
   * Returns whether the game is active. An active game has
   * a word selected and is not lost or won.
   * @returns {boolean} True if the game is active, false otherwise.
   */
  get isActive() {
    return this.hiddenWord.length > 0 && !this.isWon && !this.isLost;
  }

  /**
   * Returns the number of guesses.
   * @returns {number} The number of guesses.
   */
  get guesses() {
    return this.#guesses;
  }

  /**
   * Guesses a letter. All matching locations (if there are any)
   * for the letter in the hidden word will be revealed.
   * @param {string} letter - The letter to guess.
   * @returns {boolean} True if the guess was correct, false otherwise.
   * @throws {Error} If the player has no guesses remaining.
   */
  guess(letter) {
    if (this.#guesses <= 0) {
      throw new Error("No guesses remaining");
    }

    this.#word.forEach((e, i) => {
      if (e === letter) {
        this.#hiddenWord[i] = e;
      }
    });

    if (this.#hiddenWord.includes(letter)) {
      return false;
    }

    this.#guesses--;
    return true;
  }

  /**
   * Fetches n random words from a word API.
   * @param {number} [n=100] - The number of words to pull.
   * @returns {Promise<string[]>} A promise resolving to an array of words.
   * @throws {Error} If word fetching fails.
   */
  async #fetchWords(n = 100) {
    const response = await fetch(`${this.#apiUrl}?number=${n}`);

    if (!response.ok) {
      throw new Error(response.statusText);
    }

    return response.json();
  }
}

index.mjs (this could just as easily be browser code--hangman.mjs is purely logic and supports either client):

import readline from "readline";
import Hangman from "./hangman.mjs";

const rl = readline.createInterface({
  input: process.stdin,
  output: process.stdout,
});

const prompt = (question) => {
  return new Promise((resolve) => {
    rl.question(question, (answer) => {
      resolve(answer);
    });
  });
};

(async () => {
  const hangman = new Hangman();

  for (;;) {
    await hangman.newGame();

    while (hangman.isActive) {
      console.log("The word", hangman.hiddenWord);
      console.log("Guesses left", hangman.guesses);
      const letter = await prompt("Guess a letter: ");
      hangman.guess(letter);
    }

    console.log(`Game over! You ${hangman.isWon ? "won" : "lost"}`);
    console.log(`The word was ${hangman.revealedWord}`);
  }
})();
\$\endgroup\$
1
  • \$\begingroup\$ Wow. I've been coding JS for almost 4 years (age 14). That's a lot of improvements! Thank you. \$\endgroup\$ Apr 14 at 0:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.