7
\$\begingroup\$

I just want some feedback to improve readability and structure for this implementation of Hangman. The goal was to write it in a purely functional way. However, coming from an imperative background, I found it difficult to organize functional code.

function Hangman(word, revealed = word.replace(/\w/g, '_'), errors = 0) {
  const hasLost = errors > 5;
  const hasWon = !hasLost && word === revealed;

  const tryGuess = (letter, character, guess) =>
    new RegExp(letter, "i").test(character) ? character : '_';

  const revealCharacter = (letter, character, guess) => /_/.test(guess) ? 
    tryGuess(letter, character, guess) : guess;

  const revealLetters = (letter, [head, ...rest], [guess, ...guessed]) => {
    const character = / /.test(head) ? ' ' : revealCharacter(letter, head, guess);
    return rest.length ?
      character + revealLetters(letter, rest, guessed) : character;
  }

  const getErrorCount = (guess, word, count) =>
    new RegExp(guess, "i").test(word) ? count : count + 1;

  const getResult = () => hasLost ? 'You lose' : 'You win';

  if (!hasWon && !hasLost) {
    this.guess = letter => new Hangman(word,
      revealLetters(letter, word, revealed),
      getErrorCount(letter, word, errors));
    console.log(revealed, errors);
  } else {
    console.log(getResult());
  }
}

const game = new Hangman('Agile Development');
game.guess('e').guess('a').guess('t').guess('z');

UPDATE: Below is the revised edition of the code with the advised changes. The code base looks much cleaner and I decided to try to stick to only using primitives and functions. Having each function accomplish a single task increased the code size, but definitely improved the readability and maintainability of the code.

function Hangman(text, revealed = null, errors = 0) {
  { // constructor
    if (revealed === null) {
      return Hangman(text, revealMatch(/\s/g, text), 0);
    }

    if (isDone()) {
      console.info(getGameStatus());
    } else {
    	console.info(arguments);
      return makeGuess;
    }
  }
  
  function createMatchedCharacterList(matcher) {
    return (revealed, actual, index) => {
      if (matcher.test(actual)) {
        revealed[index] = actual;
      }
      return revealed;
    }
  }
  
  function revealMatch(matcher, text) {
    return text.split('').reduce(createMatchedCharacterList(matcher), [])
  }
  
  function revealCharacter(guess) {
    return Object.assign([], revealed, revealMatch(new RegExp(guess, 'i'), text));
  }
  
  function makeGuess(guess) {
    return (hasCharacter(guess) && !hasGuessed(guess)) ?
      Hangman(text, revealCharacter(guess), errors) : 
      Hangman(text, revealed, errors + 1);
  }
  
  function hasCharacter(character) {
    return text.search(character) > -1;
  }
  
  function hasGuessed(guess) {
    return revealed.join('').search(guess) > -1;
  }
  
  function hasLost() {
    return errors > 5;
  }
  
  function hasWon() {
    return text === revealed.join('');
  }
  
  function isDone() {
    return hasLost() || hasWon();
  }
  
  function getGameStatus() {
    return isDone() ? hasLost() ? 'You lose' : 'You win' : 'Incomplete';
  }
}

console.log(Hangman('eatz')('e')('a')('t')('z'));

\$\endgroup\$
3
\$\begingroup\$

Such a great question,

  • hasLost and hasWon should be functions
  • You should write functions with the keyword function, otherwise you get a mess of anonymous functions in stack-traces. The fat arrow syntax was only meant for inline functions.
  • head is not a great variable name, I don't know what it is supposed to be from the name
  • On the whole, the challenge in pure fp is to never assign, which you do a few times, read this for good measure: https://stackoverflow.com/a/33114654/7602
  • I am not sure I like that the guess function creates a new Hangman object every single time, there has to be a cleaner approach.

I found an interesting guide: https://drboolean.gitbooks.io/mostly-adequate-guide/ which states that this should be avoided.. Meanwhile, this is my this littered approach. It follows all my recommendations, and shows where I feel fat arrows have a place. I will try to rewrite again once I finish the guide ;)

function Hangman(word, copy, mistakes){

  //Initialize again, no assignments here ;)
  if(!copy){
    return Hangman(word, '_'.repeat(word.length), 0 );
  }

  //Non Exposed functions requiring context
  const won = (() => {
    return function won(){
      return word == copy;                             
    };                          
  })();

  const lost = (() => {
    return function lost(){
      return mistakes > 5;
    }
  })();

  const updateMistakes = (() => {
    return function updateMistakes(oldCopy , newCopy){
      return mistakes += oldCopy == newCopy ? 1 : 0;
    }
  })();

  const applyGuess = (() => {
    return function applyGuess(letter){
      return copy  = word.split('').map( (c,i) => letter == c ?  c : copy[i] ).join('');
    }
  })();

  //Exposed Functions
  function guess(){
    return function guess(letter){ 
      if(!done()){
        updateMistakes( copy , applyGuess(letter) );      
      }
      this.display();
      return this;
    };
  }

  function display(){
    return function display(){
      console.log( won()? 'You won' : lost() ? 'You lost' : copy + ' ' + mistakes );
    };
  }

  //Return object
  return {
    guess : guess(),
    display: display(),    
  }

  //Pure function?
  function done(){
    return won() || lost();
  }  

}

const game = Hangman('Agile Development');
game.guess('e').guess('a').guess('t').guess('z');

Okay, 2nd version with far less this, many more clean functions:

function Hangman(word, copy, mistakes){

  //Initialize again, no assignments here ;)
  if(!copy){
    return Hangman(word, '_'.repeat(word.length), 0 );
  }

  //Exposed Functions
  function guess(){
    return function guess(letter){ 
      if( !done( won(word,copy), lost(mistakes) ) ){
        //An assignment, it was unavoidable I think
        mistakes = updateMistakes( mistakes, copy , copy = applyGuess(word,copy,letter) );      
      }
      display(word, copy, mistakes, letter );
      return this;
    };
  }

  //Return object, kind pointless now that there is only 1 function :/
  return {
    guess : guess()
  }

  //Not entirely pure function, not cacheable
  function display(word, copy, mistakes, letter){
    console.log( won(word,copy) ? 'You won' :
                 lost(mistakes) ? 'You lost' :
                 copy + ' ' + mistakes + ' You tried (' + letter + ')' );
  }

  //Pure functions!!
  function applyGuess(word, copy, letter){
    return word.split('').map( (c,i) => letter == c ?  c : copy[i] ).join('');
  }

  function updateMistakes(mistakes, oldCopy , newCopy){
    return mistakes + (oldCopy == newCopy ? 1 : 0 );
  }

  function done(won, lost){
    return won || lost;
  }

  function won(word, copy){
    return word == copy;                             
  }   

  function lost(mistakes){
      return mistakes > 5;
  }

}

const game = Hangman('Agile Development');
game.guess('e').guess('a').guess('t').guess('z');
\$\endgroup\$
  • \$\begingroup\$ Thanks for taking the time and analysis. Without the this, it's great that the game now can be instantiated without being newed. Why are methods like lost in self-executing function calls? Is this a stylistic thing or a JavaScript thing? \$\endgroup\$ – Pete Sep 8 '16 at 18:45
  • \$\begingroup\$ That's to create the closure, though I am sure this can be done better. \$\endgroup\$ – konijn Sep 8 '16 at 19:05
  • \$\begingroup\$ I just tested creating a function called won inside Hangman instead of assigning it to a const and the closure still works fine. So just using plain functions might be cleaner. \$\endgroup\$ – Pete Sep 8 '16 at 20:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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