Karma Sutra game

Question

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();
})();

• Phrancis: That's a neat idea for a game!
• 200_success: Is the misspelling as Karma Sutra intentional?
• Ryan Mc: @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: @Phrancis thanks. Definitely got room for improvement. This is very much a beta version.

Answer 1

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

• Ryan Mc: 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: 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
• woxxom: 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.
• Ryan Mc: 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