Karma Sutra game

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

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 '' + position +
               '';
    }).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 itera

StackExchange Code Review Q#145716, answer score: 9