# 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.

(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

// create a p element that we will display the final answer / chosen positions in

// initially don't display this element

// 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

// 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');
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;

// 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
selections = [];
femaleSelections = [];
maleSelections = [];

container.className = "female";
sectionTitle.className += ' slideInLeft';
choices.forEach(function(element){
element.disabled = false;
})  ;
femaleSelected.classList.remove('selected', 'animated', 'pulse', 'inifinte');
maleSelected.classList.remove('selected', 'animated', 'pulse', 'inifinte');
});

}
}

});

}

}

// call the function
getSelection();

})();

• That's a neat idea for a game! – Phrancis Oct 31 '16 at 5:59
• Is the misspelling as Karma Sutra intentional? – 200_success Oct 31 '16 at 14:34
• @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. – Ryan Mc Nov 1 '16 at 5:00
• @Phrancis thanks. Definitely got room for improvement. This is very much a beta version. – Ryan Mc Nov 1 '16 at 5:01

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
• 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');

// create positions
'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.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);

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';

choices.gents = [];
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;

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

if (choices.gents.length == MAX_CHOICES) {
}
}
}

• Return from function soon instead of having a huge else clause or rework it (extract)

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

// 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);

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

answer.textContent = "Looks like you'll be doing " +
ladiesChoice.textContent + ' and ' +
gentsChoice.textContent +
' tonight!';
}

• 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) {
}
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
• 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. – Ryan Mc Nov 1 '16 at 4:59
• 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 – Ryan Mc Nov 1 '16 at 9:47
• 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. – wOxxOm Nov 1 '16 at 9:51
• 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 – Ryan Mc Nov 1 '16 at 10:39