8
\$\begingroup\$

I have created a simple Javascript game and want some second opinions on the code. How can it be done more efficiently for example?

Please see the code below, plus a link to a live working version on github.

Code to github

Thanks in advance.

(function(){

// The intro element. This element displays some introductory text and a button to start 
// playing
var intro = document.getElementById("intro");

// hide the listcontainer at first
var listContainer = document.getElementById('positions');
listContainer.style.display = 'none';

var sectionTitle = document.getElementById("section-title");
sectionTitle.style.display = 'none';

// the button to start the game
var displayPositions = document.getElementById("playButton");

// when we click the display button, we hide the intro element then display the
// list of options and section titles
displayPositions.onclick = function() {
    intro.style.display = 'none';
    sectionTitle.style.display = 'block';
    listContainer.style.display = 'block';
};

/*
 * Create a function to display all positions
 */
function createPositions() {

    // the array of positions
    var positions = [
        "Sixty-Nine",
        "Missionary",
        "Doggy style",
        "Rodeo",
        "Reverse Cowgirl",
        "Girl on top",
        "Zeus",
        "Venus",
        "Workout"
    ];

    // get hold of the list container on the page
    var listContainer = document.getElementById('positions');

    // loop through all the positions and place each one in an li element 
    for(var i = 0, n = positions.length; i < n; i++) {

        // create an li element
        var listElement = document.createElement("button");

        // set the attribute and ID to be the position index
        var listValue = listElement.setAttribute("value", "position-" + [i]);
        listElement.setAttribute("id", "position-" + [i]);
        listElement.setAttribute("class", "choice-buttons");

        // create the text to be placed in the element
        var listContent = document.createTextNode(positions[i]);

        // append the text to the li 
        listElement.appendChild(listContent);

        // append the li to the ul 
        listContainer.appendChild(listElement);
    }
}

// call function 
createPositions();

/*
 * Create a function that gets the selections 
 * from the user
 */
function getSelection() {

    // create an empty array
    var selections = [];
    var femaleSelections = [];
    var maleSelections = [];

    // get the main container of the page
    var container = document.getElementById("main-container");

    // get this again to access the dom element
    var listContainer = document.querySelector('#positions');

    // get the element where we put the answer. This is where we will see the final results
    var answerElement = document.getElementById("chosenPositions");
    var answerP = document.createElement("P");

    // create a p element that we will display the final answer / chosen positions in
    answerElement.appendChild(answerP);

    // initially don't display this element
    answerElement.style.display = 'none';

    // start out with a default class
    container.className = "female";

    // store the choices as an array
    var choices = listContainer.querySelectorAll('button');

    // get the length of the choices so we can loop over all of them                
    var choicesLength = choices.length;

    // start loop 
    for(var i = 0; i < choicesLength; i++) {

        // for each element in the array, add an event listener to the
        // button which is clicked
        choices[i].addEventListener("click", function(){

            // the position, as in "sexual" position, is the value of the 
            // current element / button
            var position = this.getAttribute("value");

            // push that position into the selections array
            // selections.push(position);

            // for debugging - TODO: delete this:
            // console.log(selections);


            // First get the female selections
            if(femaleSelections.length < 2) {

                // push them into the femaleSections array
                femaleSelections.push(position);
                this.disabled = true;

                // DEBUG: 
                console.log(femaleSelections);

                // change the classname to male once female selections reach 2
                if(femaleSelections.length == 2) {
                    container.className = "male";
                    sectionTitle.innerHTML = 'Gents';
                    sectionTitle.classList.remove('slideInLeft');
                    sectionTitle.className += ' slideInRight';
                    selections.push(femaleSelections);
                }
            }

            // now get male selections
            else if(maleSelections.length < 2) {

                // push them into the maleSelections array
                maleSelections.push(position);
                this.disabled = true;

                // DEBUG: 
                console.log(maleSelections);

                // change the classname to results once the male sections reach 2
                // and disable all the buttons so no more selections can be made
                if(maleSelections.length == 2) {
                    container.className = "results";
                    sectionTitle.innerHTML = 'Results';
                    sectionTitle.classList.remove('slideInRight');
                    sectionTitle.className += ' tada';
                    selections.push(maleSelections);

                    // randomly get a selection from each array                 
                    var randomSelectionF = Math.floor(Math.random() * femaleSelections.length);
                    var randomSelectionM = Math.floor(Math.random() * maleSelections.length);

                    // get the male and female positions as text
                    var femalePosition = document.getElementById(femaleSelections[randomSelectionF]).innerHTML;
                    var malePosition = document.getElementById(maleSelections[randomSelectionM]).innerHTML;

                    // get the actually button we clicked on so we can add classes to it
                    var femaleSelected = document.getElementById(femaleSelections[randomSelectionF]);
                    var maleSelected = document.getElementById(maleSelections[randomSelectionM]);

                    femaleSelected.className += ' selected animated infinite pulse';
                    maleSelected.className += ' selected animated infinite pulse';

                    // if the two randmon choices are the same position, manually choose them.
                    // NOTE: I'm not sure if this is the best approach to this? 
                    if(femaleSelections[randomSelectionF] === maleSelections[randomSelectionM]) {
                        femalePosition = document.getElementById(femaleSelections[0]).innerHTML;
                        malePosition = document.getElementById(maleSelections[1]).innerHTML;
                    }

                    // Create the decision string, ie what your final positions will be     
                    var decision = femalePosition + " and " + malePosition;

                    // display the answer element                       
                    answerElement.style.display = 'block';

                    // Put the answer into the HTML element
                    answerP.appendChild(document.createTextNode("Looks like you'll be doing " + decision + " tonight!"));

                    // disable all the buttons so no more selections can be made
                    choices.forEach(function(element){
                        element.disabled = true;
                    })  ;

                    // get hold of the play again button
                    var playAgain = document.getElementById("startOver");

                    // to play again we reset all out variables to their
                    // original values 
                    playAgain.addEventListener("click", function(){
                        selections = [];
                        femaleSelections = [];
                        maleSelections = [];

                        container.className = "female";
                        sectionTitle.innerHTML = 'Ladies';
                        sectionTitle.classList.remove('tada');
                        sectionTitle.className += ' slideInLeft';
                        answerElement.style.display = 'none';
                        choices.forEach(function(element){
                            element.disabled = false;
                        })  ;
                        answerP.innerHTML = '';                 
                        femaleSelected.classList.remove('selected', 'animated', 'pulse', 'inifinte');
                        maleSelected.classList.remove('selected', 'animated', 'pulse', 'inifinte');
                    });

                }
            }

        });

    }       

}

// call the function
getSelection();

})();
\$\endgroup\$
  • \$\begingroup\$ That's a neat idea for a game! \$\endgroup\$ – Phrancis Oct 31 '16 at 5:59
  • \$\begingroup\$ Is the misspelling as Karma Sutra intentional? \$\endgroup\$ – 200_success Oct 31 '16 at 14:34
  • \$\begingroup\$ @200_success completely by the by, but I'll answer it as I'm not a complete dick (well, almost). I honestly thought that was how you spell it. I think westerners spell it like that anyway. \$\endgroup\$ – Ryan Mc Nov 1 '16 at 5:00
  • \$\begingroup\$ @Phrancis thanks. Definitely got room for improvement. This is very much a beta version. \$\endgroup\$ – Ryan Mc Nov 1 '16 at 5:01
9
\$\begingroup\$

Adding feedback in the form of hints like Select the first position, then Now select the second position would enhance UX.

Now let's nitpick.

  • Split the huge multi-purpose function into smaller specific ones, group code by purpose
  • Most of your comments are redundant and just add infonoise to self-explanatory code
  • Use constants instead of magic values e.g. MAX_CHOICES instead of 2
  • Instead of meaningless array in "position-" + [i] simply use "position-" + i

// Initialization
var intro = getByID('intro');
var container = getByID('main-container');
var choicesContainer = getByID('positions');
var sectionTitle = getByID('section-title');
var answer = getByID('chosenPositions');

// create positions
choicesContainer.insertAdjacentHTML('beforeend', [
        'Sixty-Nine', 'Missionary',      'Doggy style',
        'Rodeo',      'Reverse Cowgirl', 'Girl on top',
        'Zeus',       'Venus',           'Workout'
    ].map(function(position, i) {
        return '<button id="position-' + i + '" ' +
                       'class="choice-buttons">' + position +
               '</button>';
    }).join('')
);

var MAX_CHOICES = 2;
var choices = choicesContainer.querySelectorAll('button');
choices.ladies = [];
choices.gents = [];

getByID('playButton').onclick = play;
getByID('startOver').onclick = restart;
choices.forEach(function(c) { c.onclick = choose; });

// only intro is shown on start
hide(choicesContainer);
hide(sectionTitle);
hide(answer);

// ladies choose first
container.className = 'female';
  • Use properties like .className directly instead of setAttribute when creating elements, the same applies to .value instead of getAttribute
  • Use .textContent instead of creating a separate text node

// overall game control
function play() {
    hide(intro);
    show(sectionTitle);
    show(choicesContainer);
}

function restart() {
    container.className = 'female';
    sectionTitle.textContent = 'Ladies';
    sectionTitle.classList.remove('tada');
    sectionTitle.classList.add('slideInLeft');

    hide(answer);

    choices.ladies = [];
    choices.gents = [];
    deanimateChoice(ladiesChoice);
    deanimateChoice(gentsChoice);
    choices.forEach(function(c) { element.disabled = false; });
}
  • Extract the huge click handler to a separate function instead of burying it inside a loop or move out the variables to group them with the other globally used ones.

// in-game control
function choose() {
    // prevent duplicate choices
    this.disabled = true;

    // Ladies first
    if (choices.ladies.length < MAX_CHOICES) {
        choices.ladies.push(this.id);

        // change the classname to male once female choices reach 2
        if (choices.ladies.length == MAX_CHOICES) {
            container.className = 'male';
            sectionTitle.textContent = 'Gents';
            sectionTitle.classList.remove('slideInLeft');
            sectionTitle.classList.add('slideInRight');
        }
    } else {
        choices.gents.push(this.id);

        if (choices.gents.length == MAX_CHOICES) {
            showAnswer();
        }
    }
}
  • Return from function soon instead of having a huge else clause or rework it (extract)

// results
function showAnswer() {
    // change the classname to results once the male sections reach 2
    container.className = 'results';
    sectionTitle.textContent = 'Results';
    sectionTitle.classList.remove('slideInRight');
    sectionTitle.classList.add('tada');

    // and disable all the buttons so no more choices can be made
    choices.forEach(function(c) { c.disabled = true; });

    // randomly get a selection from each array                 
    do {
        var ladiesIndex = Math.floor(Math.random() * MAX_CHOICES);
        var gentsIndex = Math.floor(Math.random() * MAX_CHOICES);
    } while (ladiesIndex == gentsIndex);

    var ladiesChoice = getByID(choices.ladies[ladiesIndex]);
    var gentsChoice = getByID(choices.gents[gentsIndex]);
    animateChoice(ladiesChoice);
    animateChoice(gentsChoice);

    answer.textContent = "Looks like you'll be doing " + 
        ladiesChoice.textContent + ' and ' +
        gentsChoice.textContent +
        ' tonight!';
    show(answer);
}
  • Add a function to get elements by id since you're using it a lot e.g. id('str') or $('str') and use it consistently (currently you have one querySelector).
  • Add functions show(element) as well as hide that alter .style.display to up readability

// utilities
function getByID(ID) {
    return document.getElementById(ID);
}
function show(element) {
    element.style.display = 'block';
}
function hide(element) {
    element.style.display = 'none';
}
function animateChoice(element) {
    element.classList.add('selected', 'animated', 'infinite', 'pulse');
}
function deanimateChoice(element) {
    element.classList.remove('selected', 'animated', 'pulse', 'inifinte');
}
  • Use classList methods consistently, don't mix with direct className modifications.
  • Don't cache array length in for-loops if you don't iterate thousands or millions of times: nano-optimizations like these reduce readability
  • Add event handlers consistently: either .onxxx or .addEventListener
  • Pay attention to variable names: listContainer was redefined in nested scope.
  • Don't needlessly declare variables in the top scope as it implies they would be used later. For example, displayPositions isn't needed, instead use id("playButton").onclick =
  • Use single or double quotes consistently
\$\endgroup\$
  • \$\begingroup\$ Thanks for taking the time to review this, some pretty awesome suggestions there. I'll fork the code and try to get a working implementation then share it again. \$\endgroup\$ – Ryan Mc Nov 1 '16 at 4:59
  • \$\begingroup\$ there is one problem cropped with this code, in the restart() function you use deanimateChoice(ladiesChoice); However, ladies choice is not defined here and so won't work. Any idea how to work around this? Thanks \$\endgroup\$ – Ryan Mc Nov 1 '16 at 9:47
  • \$\begingroup\$ Oops, I guess the easiest solution would be to declare these variables in the main (top) context. I have spotted a typo as well: choices.forEach(function(c) { element.disabled = false; }); should use c, not element. \$\endgroup\$ – wOxxOm Nov 1 '16 at 9:51
  • \$\begingroup\$ Yes, I spotted the typo. Easy mistake to make. I have refactored the code here: codereview.stackexchange.com/questions/145821/… Let me know if my approach was ok or if it could be better \$\endgroup\$ – Ryan Mc Nov 1 '16 at 10:39

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.