2
\$\begingroup\$

I would love to get some feedback on my implementation of "Simon Says." I have not followed the original in that I have created a new sequence each time, rather and adding a new item to the pattern.

I'm particularly interested in the overall structure of my JS/JQuery - did I make good choices for the methods, is there enough separation of concerns etc. Are there any obvious conceptual errors?

I'm trying to cross the expanse from experienced beginner to "intermediate developer".

My code is at codepen: https://codepen.io/robin-andrews/full/BPPRWm/

$( document ).ready( function() {

    var Simon = {

        // Game constants
        cacheDom: function() {}, // to be implemented
        flashSpeed: 400,
        score : 0,
        level: 1,

        // Init whole game
        init: function() {
            this.bindControls();
        },

        newRound : function(){
            this.unbindPads();
            this.bindControls();
        },

        bindControls: function() {
            // currently only start button needs event
            $( '#start-button' ).on( 'click', this.playerRound.bind( this ) );
        },

        unbindControls: function() {
            $( '#start-button' ).off( 'click' );
        },

        bindPads: function() {
            $( '#play-area' ).on( 'click', '.pad', this.checkSequence.bind( this ) ); // pass object
        },

        unbindPads: function() {
            $( '#play-area' ).off( 'click' );
        },

        playerRound: function() {
            this.unbindControls();
            $( '#info' ).text( 'Copy the sequence using mouse clicks' );
            this.computerSequence = this.generateSequence( this.level );
            this.remainingSequence = this.computerSequence.slice( 0 );
            this.displaySequence( this.computerSequence );
            this.bindPads();
        },

        // Generate random sequence for pads
        generateSequence: function( length ) {
            var seq = [];
            for ( var i = 0; i < length; i++ ) {
                seq.push( Math.floor( Math.random() * 4 ) + 1 ); // returns a random integer from 1 to 10]
            }
            return seq;
        },

        // Display sequence to player
        displaySequence: function( seq ) {

            var self = this;

            if ( seq.length >= 1 ) {

                var current_list_item = seq[ 0 ];
                $( '#pad' + current_list_item ).css( 'opacity', '1' );
                $( '#pad' + current_list_item ).animate( {
                    'opacity': '0.4'
                }, self.flashSpeed, function() {
                    self.displaySequence( seq.slice( 1 ) );
                } );

                return;
            }
        },

        checkSequence: function( e ) {

            var boxNum = $( e.target ).attr( 'id' ).slice( -1 );

            if ( boxNum != this.remainingSequence[ 0 ] ) {
                this.playerLoses();
                return; //?
            }

            this.remainingSequence = this.remainingSequence.slice( 1 );
            if ( this.remainingSequence.length < 1 ) {
                this.playerWins();
            }
        },

        playerWins : function(){
            $( '#info' ).text( 'You won. Press play to continue.' );
            this.score += this.level;
            $( '#score' ).text( 'Score: ' + this.score);
            this.level += 1;
            this.newRound();
        },

        playerLoses: function() {
            $( '#info' ).text( 'You lost. Press play to play again.' );
            this.level = 1;
            this.score = 0;
            $( '#score' ).text( 'Score: ' + this.score);
            this.newRound();
        }

    };

    Simon.init();

} );

// To do
// increae speed with level?
// Track highest score?
* {
    box-sizing: border-box;
}

body {
    margin: 0;
    background-color: grey;
    font-family: "Lucida Console", Monaco, monospace;
}

#wrapper{
    width: 600px;
    height: 400px;
    margin: auto;
    position: relative;
}

#control-area{
    width: 200px;
    height: 400px;
    background-color: black;
    float: left;
}

#start-button{
    display: block;
    width: 100px;
    margin: 20px auto;
    background-color: white;
    border: none;
    border-radius: 30px;
    padding: 5px;
    top: 200px;
    font-size: inherit;
    font-family:inherit;
    outline: none;
}

#info{
    background-color: white;
    width: 150px;
    height: 90px;
    padding: 5px;
    margin: 20px auto;
    border-radius: 5px;
}

#score{
    background-color: white;
    width: 100px;
    padding: 5px;
    margin: 20px auto;
    border-radius: 5px;
}

#play-area{
    width: 400px;
    height: 400px;
    float: right;
}

.pad{
    width: 200px;
    height: 200px;
    float: left;
    opacity: 0.4;
}

#pad1{
    background: #66ff00;
}

#pad2{
    background: red;
}

#pad3{
    background: yellow;
}

#pad4{
    background: blue;
}
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Robin Says</title>
    <link rel="stylesheet" href="style.css">
    <script
    src="https://code.jquery.com/jquery-3.3.1.min.js"
    integrity="sha256-FgpCb/KJQlLNfOu91ta32o/NMZxltwRo8QtmkMRdAu8="
    crossorigin="anonymous"></script>
    <script src="script.js"></script>
</head>
<body>
    <div id="wrapper">
        <h1 id="title">Robin Says</h1>
        <div id="control-area">
            <p id="info">
                Press start to play
            </p>
            <div id="score">Score: 0</div>
            <button id="start-button">Start</button>
        </div>
        <div id="play-area">
            <div id="pad1" class="pad"></div><!-- Could use data attributes -->
            <div id="pad2" class="pad"></div>
            <div id="pad3" class="pad"></div>
            <div id="pad4" class="pad"></div>
        </div>
    </div>
</body>
</html>

\$\endgroup\$
  • \$\begingroup\$ Note that the code you posted here is a bit different from your CodePen (the button is labeled "Start" rather than "Play"). \$\endgroup\$ – 200_success Aug 5 '18 at 17:49
1
\$\begingroup\$

Separate game for unrelated stuff

The core of a game should be independent of the user interface. It should be able to work using the DOM or a console. It could be sounds sequences, words, shapes or anything. As long as the external interfaces implement the required properties and the callback get the correct data you should not need to change any of the game code to change how its played.

Your enquirers

did I make good choices for the methods,

No, to pick on just on

simon.init not needed. Should have one starting point and as a game it should relate to the game. simon.newGame would have been better. All the init can be done outside the object simon

See rewrite for an alternative approch.

is there enough separation of concerns etc.

No

The following methods are unrelated to the game.

    bindControls: function() {
    unbindControls: function() {
    bindPads: function() {
    unbindPads: function() {

    // Related but doing too much
    displaySequence : function() {

The Simon object should be able to function without the DOM. It creates sequences and outputs them, it then compares user input against the sequence, scores and moves to the next level or restarts.

It interfaces with the outside state by providing functions to handle UI events and uses a defined interface to control the systems display state. It should not be creating listeners, it should not be querying the DOM, or getting involved in the detailed animation of display items. All it does is play Simon Says

Are there any obvious conceptual errors?

Yes

  • Simon is too busy with unrelated tasks.
  • Create separate interfaces to handle display and setup UI.
  • Games are state machines, and thus should have a way to change state that is robust (not reliant on current state) and easy to understand. Your code makes it vary hard to change between states as it is reliant on the current state to be able to change to the next. Calling a function while the object is not an appropriate state will cause the game to crash.
  • You should not fill your code with magic numbers and names. Use a settings object to define all the variables in one place.

Rewrite example

The following is an example of how it could be written.

As we can not use modules in these snippets much of what would be private variables are as function scoped variables shared between all. In the real world they would only be global to related code or available for import.

I could not see any need for jQuery (particularly its very poor animation handling) and did not use it at all. However jQuery is a personal choice.

(Note slight game play change, next level starts automatically)

//addEventListener("load", ()=> {
"use strict";
/*===== Utilities not directly related to the question =====*/
const callEach = (items, name, ...args) => { for(const item of items) { item[name](...args) } };
const setEach = (items, name, value) => { for(const item of items) { item[name] = value } };
const setOf = (count, cb = (i)=>i) => {var a = [],i = 0; while (i < count) { a.push(cb(i ++)) } return a };
const randShuffle = (a, l = a.length) => { while (l) { a.push(a.splice(Math.random() * (l--) | 0, 1)[0]) } return a };
const ease = v => v < 0 ? 0 : v > 1 ? 1 : v ** 2; 
const query = query => document.querySelector(query);

/*=========== All game related constants =====================*/
const settings = {
    autoStart : true,  // if true next level plays auto
    autoStartPause : 1000, // Time to wait before auto start
    endLevelPause : 1500, // in ms
    text : {
        score : "Score: ##", // The ## is replaced with the score,
        instructions : "Repeat using sledge hammer (gently though)",
        lost : "You lost.",
        won : "You won.",
        continue : "Click Start to continue.",
        start : "Click Start to play.",
        showingSequence : "Simon says..",
    }, 
    pad : {
        flashTime : 600, // in ms
        offOpacity : 0.4,
        onOpacity : 1,
    }
};

/*====Get refereneces to the DOM intergaces needed====*/
const UI = {
    start : query("#start-button"),
    pads : query("#play-area"),
    info : query("#info"),
    score : query("#score"),
};

/*==== All game states ====*/
const states = {
    showingSequence : 1,
    waitingForStart : 2,
    waitingForUserSequence : 3, // waiting for the player to input sequence
    pause : 4,                 // this state is always followed by waitingForStart
    autoStart : 5,
};    

/*= Variables. Normaly these would be with the related code, but as the demo can not use modules 
    they are gust globals */
var score, level, sequence, sequencePos, gameState, globalTime = 0;        


/*== Handles pad animation. call flash to flash a padd, Give it a callback to call when flash complete */
function pad(element) {
    var offAt = -1, onflashoff, on = true; // init state so that pad goes from on to off state
    return {
        update() {
            var opacity;
            if (on) {
                if (offAt === undefined) { offAt = globalTime + settings.pad.flashTime }
                const time = (offAt - globalTime) / settings.pad.flashTime;
                opacity = ease(time) * (settings.pad.onOpacity - settings.pad.offOpacity) + settings.pad.offOpacity;
                if (time <= 0) {
                    on = false;
                    offAt = undefined;
                    opacity = settings.pad.offOpacity;
                    if (onflashoff) { onflashoff() }
                }
                element.style.opacity = opacity;
            }
        },
        flash() { 
            offAt = undefined;
            on = true;
        },
        set onflashoff(callback) {
            if (typeof callback === "function") { onflashoff = callback }
            else { onflashoff = undefined }
        } 
    }
}
// Again as there are not modules this is just in global scope 
const pads = [...UI.pads.querySelectorAll(".pad")].map(padElement => pad(padElement));
// controls the pads 
function animations(time) {
    globalTime = time;
    callEach(pads, "update");
    requestAnimationFrame(animations);
}
requestAnimationFrame(animations);



    
const simon = {
    set gameState(state) {
        // block states changes that are not expected
        if(gameState === states.autoStart && state !== states.showingSequence) { return }
        if(gameState === states.pause && state !== states.waitingForStart) { return }
        
        
        gameState = state;
        switch (state) {
            case states.waitingForStart:
                simon.info = level === 1 ? settings.text.start : settings.text.continue;
                if (settings.autoStart && level > 1) { simon.gameState = states.autoStart }
                break;             
            case states.autoStart:
                setTimeout(() => simon.gameState = states.showingSequence, settings.autoStartPause);
                break;
            case states.waitingForUserSequence:
                simon.info = settings.text.instructions;
                sequencePos = 0;
                break;
            case states.showingSequence:
                simon.generateSequence();
                simon.info = settings.text.showingSequence;
                break;
            case states.pause:
                setTimeout(() => simon.gameState = states.waitingForStart, settings.endLevelPause);
                break;
        }
    },
    set score(add) {  // zero reset score else score added to
        if (add === 0) {
            simon.level = 0;
            score = 0;
        } else {
            score += add;
            simon.level = 1;
        }
        UI.score.textContent = settings.text.score.replace("##",score);
    },
    set level(add) { level = add === 0 ? 1 : level + add }, // zero resets else adds to
    set info(text) { UI.info.textContent = text },        
    
    generateSequence() { 
        const r = Math.random() * pads.length | 0;
        sequence = randShuffle(setOf(level, i => (i + r) % pads.length));
        sequencePos = 0;
        pads[sequence[sequencePos]].flash();
    },
    nextPad() {
        if (gameState === states.showingSequence) {
            sequencePos ++;
            if(sequencePos < sequence.length){
                pads[sequence[sequencePos]].flash();
            }else{
                simon.gameState = states.waitingForUserSequence;
            }
        }
    },
    startClicked() {
        if (gameState === states.waitingForStart) {
            simon.gameState = states.showingSequence;                
        }
    },
    padClicked(event) {
        if (gameState === states.waitingForUserSequence) {
            const padId = +event.target.dataset.pad;
            if (padId === sequence[sequencePos]) {
                pads[padId].flash();
                sequencePos ++;
                if (sequencePos === sequence.length) {
                    simon.win();
                }
            } else {
                simon.lost();
            }
        }
    },
    win(){
        simon.info = settings.text.won;
        simon.score = level;
        simon.gameState = states.pause;
    },
    lost() {
        simon.info = settings.text.lost;
        simon.score = 0;
        simon.gameState = states.pause;
    }
};

/* bind UI and display related stuff to Simon */
UI.start.addEventListener("click", simon.startClicked);
UI.pads.addEventListener("click", simon.padClicked);
setEach(pads, "onflashoff", simon.nextPad);

/* Clear the score. This also set the level to 1*/
simon.score = 0;
/* Set the starting state. */
simon.gameState = states.waitingForStart;
//} );
* {
    box-sizing: border-box;
}

body {
    margin: 0;
    background-color: grey;
    font-family: "Lucida Console", Monaco, monospace;
}

#wrapper{
    width: 600px;
    height: 400px;
    margin: auto;
    position: relative;
}

#control-area{
    width: 200px;
    height: 400px;
    background-color: black;
    float: left;
}

#start-button{
    display: block;
    width: 100px;
    margin: 20px auto;
    background-color: white;
    border: none;
    border-radius: 30px;
    padding: 5px;
    top: 200px;
    font-size: inherit;
    font-family:inherit;
    outline: none;
    cursor : pointer;

}

#info{
    background-color: white;
    width: 150px;
    height: 90px;
    padding: 5px;
    margin: 20px auto;
    border-radius: 5px;
}

#score{
    background-color: white;
    width: 100px;
    padding: 5px;
    margin: 20px auto;
    border-radius: 5px;
}

#play-area{
    width: 400px;
    height: 400px;
    float: right;
}

.pad{
    width: 200px;
    height: 200px;
    float: left;
    opacity: 0.4;
    cursor : pointer;
}

.pad--color-green{
    background: #66ff00;
}

.pad--color-red{
    background: red;
}

.pad--color-yellow{
    background: yellow;
}

.pad--color-blue{
    background: #0099FF;
}
<div id="wrapper">
    <h1 id="title">Simon Says</h1>
    <div id="control-area">
        <p id="info"> </p>
        <div id="score">Score: 0</div>
        <button id="start-button">Start</button>
    </div>
    <div id="play-area">
        <div class = "pad pad--color-green"  data-pad = "0"></div>
        <div class = "pad pad--color-red"    data-pad = "1"></div>
        <div class = "pad pad--color-yellow" data-pad = "2"></div>
        <div class = "pad pad--color-blue"   data-pad = "3"></div>
    </div>
</div>

\$\endgroup\$
  • \$\begingroup\$ Thanks @Blindman67. Lots to think about there. I wonder if you could describe the file structure if this code was "in the real world" using modules etc? \$\endgroup\$ – Robin Aug 6 '18 at 14:39
  • \$\begingroup\$ @Robin The link is a good starting point, and has references to additional information. developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/… \$\endgroup\$ – Blindman67 Aug 6 '18 at 15:05

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.