2
\$\begingroup\$

I wrote a procedural version of Hangman (very basic) and would appreciate some feedback on the following points:

  • Is the basic logic OK? Any obvious improvements?
  • Any details or matters of style I need to address?
  • Var vs Const vs Let - I'm probably all over the place with this. My basic approach was "If it needs to be global, use var, if it's not going to change, use Const and if it is going to change but is not global, use Let."

I'm aware that a procedural approach may not be the best, but I wanted to focus on it for this program.

HTML:

<button id="play">Play</button>

JS:

var secretWord;
var playing = false;
var wrongGuesses;
var limit = 4;
var solutionSoFar;

const words = [
    "fish",
    "cat",
    "chicken",
    "freedom",
    "turnip"
];

function displayBoard() {
    console.log( solutionSoFar.join( " " ) );
}

function playAgainDialogue() {
    if ( confirm( "Play again?" ) ) {
        console.log( "" );
        play();
    }
    console.log( "Bye!" );
}

function play() {

    playing = true;

    // Set secret word
    secretWord = words[ Math.floor( Math.random() * words.length ) ];

    wrongGuesses = 0;

    // initialize solution
    const numLetters = secretWord.length;
    solutionSoFar = new Array( numLetters ).fill( "_" );

    // Display instructions
    console.log( "Guess one letter at a time to reveal the secret word." );

    while ( playing ) {
        displayBoard();
        playerTurn();
    }

}

function playerTurn() {

    // Out of turns
    if ( wrongGuesses >= limit ) {
        console.log( "You have used all your guesses. Game Over." );
        playing = false;
        playAgainDialogue();
        return;
    }

    // Player wins
    if ( solutionSoFar.join( "" ) === secretWord ) {
        console.log( "Yay, you guessed it!" );
        playing = false;
        playAgainDialogue();
        return;
    }

    // get player guess
    let remainingGuesses = limit - wrongGuesses;
    let playerGuess = prompt( "Guess a letter. You have " + remainingGuesses + " remaining guesses" ).toLowerCase();
    if ( secretWord.indexOf( playerGuess ) === -1 ) {
        console.log( "That letter in not in the secret word." );
        wrongGuesses += 1;
        return;
    }

    // Successful guess
    console.log( "Well done." );
    solutionSoFar.forEach( function( val, index ) {
        if ( secretWord.split( "" )[ index ] === playerGuess ) {
            solutionSoFar[ index ] = playerGuess;
        }
    } );
}

document.getElementById("play").addEventListener("click", play);
\$\endgroup\$
1
\$\begingroup\$

Logic

Is the basic logic OK? Any obvious improvements?

Well does the game work? If so then the basic logic must be OK.

Is there room for improvement, yes.

The code is tangled mess of roles and states.

  • You have 4 functions that manage 7 distinct game states.
  • There is some repeated code (win and lose).
  • You have magic values in the code (text prompts and displays)
  • The whole thing runs in a continuous blocking state which is VERY bad for browser based code (you can not exit until end game)
  • There is no checking for repeated character guesses, or valid character guesses.
  • You randomly pick words, not checking if a new game is using an already used word.

Example

The example shows some alternative approaches to creating game like state machines.

Note that I do not use the console, prompt, or confirm, thus the game needs an additional wait state to wait for input.

Nor do I vet input for repeats, or bad characters.

update I have fixed the inplay state to only allow start when not inplay

Some points.

  • Uses finite state model to control the game.
  • Settings out of the code and grouped in one place to allow easy changes.
  • Encapsulate the game in a singleton (to keep the global scope clean)
  • Though I do not use prompt and confirm, the state changes use setTimeout to change between states, this stops the game from blocking the page.
  • States are pedantic but there roles are clear and without ambiguity.

"use strict";  // Always add this to the top of your code.


/* Start game is external state request */
document.getElementById("play").addEventListener("click", ()=> {
    hangman.state = "start";
})


const hangman = (()=> {
    const settings = {
        limit : 4,
        words : ["fish","cat","chicken","freedom","turnip"],
        text : {
            start : "Guess one letter at a time to reveal the secret word.",
            prompt : "Guess a letter. You have ## remaining guesses", // ## is replaced with remaining guesses
            good : "Well done.",
            bad : "That letter in not in the secret word.",
            win : "Yay, you guessed it!",
            lose : "You have used all your guesses. Game Over.",
            again : "Play again Y/[N] ",
            gameOver : "You have used up all the words. GAME OVER!",
            exit : "Bye!",
        }
    };
    
    const display = text => log(settings.text[text] ? settings.text[text] : text);
    var word, player, inplay, wrongGuesses;
    

    
    function start() {
        if(settings.words.length > 0){
            word = settings.words.splice(Math.random() * settings.words.length | 0, 1)[0];
            player = "";
            wrongGuesses = 0;
            display("start");
            display("Cheat >>> " + word + " <<<<");
            return states.guess;
        }
        return states.gameOver;
    }
    
    function getPlayerGuess() {
        const gotKey = (key) => {
          const guess = key.toLowerCase();
          player += guess;
          game.state = word.indexOf( guess ) === -1 ? states.badGuess : states.goodGuess;
        }
        var remaining = settings.limit - wrongGuesses;
        const guess = prompt(settings.text.prompt.replace("##", remaining), gotKey);
        return states.wait;
    }    

    function checkPlayer(){
        var wordDisplay = ""; 
        for (const char of word) { wordDisplay += player.indexOf(char) > -1 ? char : "_" }
        display(wordDisplay);
        return wordDisplay === word ? states.win : states.guess;
    }
    
    const states = { // All game states named
        start : 1,
        guess : 2,
        badGuess : 3,
        goodGuess : 4,
        win : 5,
        lose : 6,
        playAgain : 7,
        gameOver : 8,
        wait : 9,
    };

    const game = {  // assign property game.state to change state
        set state(value) { 
            if(! inplay){
                if(value === "start") {
                    value = states.start;
                } else {
                    return;
                }
            }
            inplay = true; // prevents game restart while in play     
            var nextState; // if left undefined then game ends
            switch(value) {
                case states.wait:
                   return;
                case states.playAgain:
                    confirm(settings.text.again, result => {
                       game.state = result ? states.start : states.exit;
                    }); 
                    nextState = states.wait;
                    break;
                case states.start:
                    nextState = start();
                    break;
                case states.guess:
                    nextState = getPlayerGuess();
                    break;
                case states.goodGuess:
                    display("good");
                    nextState = checkPlayer();
                    break;
                case states.badGuess:
                    display("bad")
                    wrongGuesses += 1;
                    nextState = wrongGuesses === settings.limit ? states.lose : states.guess;
                    break;
                case states.win:
                    display("win");
                    nextState = states.playAgain;
                    break;                
                case states.lose:
                    display("lose");
                    nextState = states.playAgain;
                    break;
                case states.gameOver:
                    display("gameOver");   
                    break;
                default :
                    return;
            }
            if (nextState) { setTimeout(() => game.state = nextState, 0) }
            else { 
                display("exit");
                inplay = false; 
            }
        }
    };
    return game;
})();






















/*========================================================================*/
/*========== unrelated code to handle input and display ==================*/
var prompting;
var keyVal;
document.addEventListener("keyup",e=>{
    if(prompting){
        keyVal = e.key;
        prompting = false;
    }
})
function prompt(text,cb){
    var ticker = 0;
    const line = log(text );
    prompting = true;
    function tick(){
        if(prompting){
            line.textContent = text + (ticker++ & 1 ? "?" : ".");
            setTimeout(tick, 250);
        }else{
            line.textContent = text + "! " + keyVal;
            cb(keyVal[0]);
        }
    }
    tick();
}
function confirm(text, cb){
   prompt(text,(key)=>{ cb(key.toLowerCase() === "y") });
}
function log(text){
    var line;
    if(textContainer.children.length > 7){ line = textContainer.removeChild(textContainer.querySelector(".text")) }
    else{ line = Object.assign(document.createElement("div"),{className : "text"}) }
    textContainer.appendChild(line)
    line.textContent = text;
    return line;
}
body {
   background : black;
   font-size : 16px;
   color : #0f0;
.text {


}
<input type="button" id="play" value="play hangman">
<code id="textContainer">
</code>

Style

Any details or matters of style I need to address?

The style is generally good. Adding spaces after opening and before closing parentheses is a little unusual console.log( solutionSoFar.join( " " ) ) it is by no means bad. The most important aspect of code style is that you use it consistently, which you have done.

Comments should be kept to a minimum, they constitute un-vetted noise. None of your comments are needed, the code is self explanatory.

Variable declaration type

var vs const vs let - ... My basic approach was "If it needs to be global, use var, if it's not going to change, use const and if it is going to change but is not global, use let."

const

OK first const it will be the most used type of variable declaration in any good code. You used it correctly for words but missed using it for limit.

For solutionSoFar which holds an array you assign a new array each time you need. There is nothing wrong with that. But it could also have been a const and it would be declared as const solutionSoFar = []; and solutionSoFar.length = secretWord.length;' followed bysolutionSoFar.fill("_");`

Doing it this way lets you keep additional references to the same array, and reduces the overall memory management overhead (in this case negligible overhead)

var V let

Using either in global scope makes no difference apart from when you can access them.

var is function scope and hoisted. The global scope can be thought of as the global function and var has scope to everything within that call. It is also hoisted to the top of the code and thus accessible at any time (however it may not yet contain a value).

let is block scoped and is not hoisted. Again the global scope like a function is contained in a block and thus let is accessible from anywhere but only after its been declared or it will throw an error (Warning let and const can not be accessed before they have been declared) .

Which to use?

The argument that block scope reduces bugs is non sequitur.

What you use is up to you. But var for global and let for functions makes no sense. I use the rule that if declared in function scope (and not a const) it should be a var

eg

function foo() {
    var funcScope;  // this is correct for function scope
    let blockScope; // This could / (should in my book) be function scope

    if (true) {
        var blockVar; // this is function scoped and should be at the top
                      // of the function
        blockScope = 0;    // Reference error blockScope is not defined
                           // The next line recreates the variable for this
                           // scope putting the name in a temporary dead zone
                           // This is a run time error and will not be 
                           // thrown when parsing even in strict mode.

        let blockScope; // this is correct and is a new instance, not the
                        // same as the previouse declared blockScope
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for that. It looks like I need to make a serious stud of states, as I haven't really considered them. There is lots of info about finite state machines on the web. Can many of these types of games be written as FSMs? \$\endgroup\$ – Robin Aug 23 '18 at 13:32
  • \$\begingroup\$ Also, I'm curious why inplay is not treated as a state within the states object. I can kind of see how it's different from the others, but in a general sense, "in play" seems to be very much a kind of state. \$\endgroup\$ – Robin Aug 23 '18 at 21:06
  • \$\begingroup\$ @Robin inplay is part of all the states. It's states.inPlay_start and so on, but there is no point adding the name to each state. However I have noticed a mistake on my part, i should be checking for inplay in the state setter not rely on external state change to check it. Will update the answer as sone as I get a chance. \$\endgroup\$ – Blindman67 Aug 23 '18 at 22:02
  • \$\begingroup\$ @Robin I have made the changes, moved the start button out of the game and ensure that from ! inplay the state can only move to start \$\endgroup\$ – Blindman67 Aug 23 '18 at 22:21

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.