Rock, Paper, Scissors in JavaScript
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
3
down vote
favorite
I'm learning javascript and want to make my code as efficient as possible. This is something I wrote a while back and I keep revising it as I learn new things. But since I'm partly OCD, I can't help but want to stop repeating myself (DRY).
Can someone please help me use Vanilla JS to make this cleaner?
Here is the final code at CodePen!
jsFiddle
// Start All the Variables //
var userChoice;
var watson;
var saying = "You chose " + userChoice + ". Watson chose " + watson;
var win = " You Win!";
var lose = " You Lose";
var tie = " We Tied";
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
// USER'S CHOICE
// *************
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
// WATSON'S CHOICE
// ***************
function checkWatson()
// generates a random number between 0-2
randomNum = Math.floor(Math.random() * 3);
// generate a random number and assign it to one of the 3 choices
if (randomNum === 0)
watson = "rock";
else if (randomNum === 1)
watson = "paper";
else
watson = "scissors";
console.log('Watson chose: ' + watson);
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
// COMPARE USER VS WATSON
// **********************
function compare()
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
cpuChoice.innerHTML = "Watson chose: " + watson;
<img class="icon" id="rock" src="https://png.icons8.com/rock/win8/64" title="Rock" width="50" height="50">
<img class="icon" id="paper" src="https://png.icons8.com/paper-filled/ios7/50" title="Paper Filled" width="50" height="50">
<img class="icon" id="scissors" src="https://png.icons8.com/scissors-filled/ios7/50" title="Scissors Filled" width="50" height="50">
<div class="wrapper">
<p id="output"></p>
</div>
<div style="border: 0px solid blue; display: block;">
<p id="myChoice"></p>
<p>- VS -</p>
<p id="cpuChoice"></p>
<hr>
<p id="verdict"></p>
</div>
<div id="winner">ðÂÂÂ</div>
<div id="loser">ð«</div>
<div id="tied">ðÂÂÂ</div>
I made this game for my SON, and he loves it BTW :)
javascript rock-paper-scissors
add a comment |Â
up vote
3
down vote
favorite
I'm learning javascript and want to make my code as efficient as possible. This is something I wrote a while back and I keep revising it as I learn new things. But since I'm partly OCD, I can't help but want to stop repeating myself (DRY).
Can someone please help me use Vanilla JS to make this cleaner?
Here is the final code at CodePen!
jsFiddle
// Start All the Variables //
var userChoice;
var watson;
var saying = "You chose " + userChoice + ". Watson chose " + watson;
var win = " You Win!";
var lose = " You Lose";
var tie = " We Tied";
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
// USER'S CHOICE
// *************
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
// WATSON'S CHOICE
// ***************
function checkWatson()
// generates a random number between 0-2
randomNum = Math.floor(Math.random() * 3);
// generate a random number and assign it to one of the 3 choices
if (randomNum === 0)
watson = "rock";
else if (randomNum === 1)
watson = "paper";
else
watson = "scissors";
console.log('Watson chose: ' + watson);
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
// COMPARE USER VS WATSON
// **********************
function compare()
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
cpuChoice.innerHTML = "Watson chose: " + watson;
<img class="icon" id="rock" src="https://png.icons8.com/rock/win8/64" title="Rock" width="50" height="50">
<img class="icon" id="paper" src="https://png.icons8.com/paper-filled/ios7/50" title="Paper Filled" width="50" height="50">
<img class="icon" id="scissors" src="https://png.icons8.com/scissors-filled/ios7/50" title="Scissors Filled" width="50" height="50">
<div class="wrapper">
<p id="output"></p>
</div>
<div style="border: 0px solid blue; display: block;">
<p id="myChoice"></p>
<p>- VS -</p>
<p id="cpuChoice"></p>
<hr>
<p id="verdict"></p>
</div>
<div id="winner">ðÂÂÂ</div>
<div id="loser">ð«</div>
<div id="tied">ðÂÂÂ</div>
I made this game for my SON, and he loves it BTW :)
javascript rock-paper-scissors
add a comment |Â
up vote
3
down vote
favorite
up vote
3
down vote
favorite
I'm learning javascript and want to make my code as efficient as possible. This is something I wrote a while back and I keep revising it as I learn new things. But since I'm partly OCD, I can't help but want to stop repeating myself (DRY).
Can someone please help me use Vanilla JS to make this cleaner?
Here is the final code at CodePen!
jsFiddle
// Start All the Variables //
var userChoice;
var watson;
var saying = "You chose " + userChoice + ". Watson chose " + watson;
var win = " You Win!";
var lose = " You Lose";
var tie = " We Tied";
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
// USER'S CHOICE
// *************
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
// WATSON'S CHOICE
// ***************
function checkWatson()
// generates a random number between 0-2
randomNum = Math.floor(Math.random() * 3);
// generate a random number and assign it to one of the 3 choices
if (randomNum === 0)
watson = "rock";
else if (randomNum === 1)
watson = "paper";
else
watson = "scissors";
console.log('Watson chose: ' + watson);
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
// COMPARE USER VS WATSON
// **********************
function compare()
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
cpuChoice.innerHTML = "Watson chose: " + watson;
<img class="icon" id="rock" src="https://png.icons8.com/rock/win8/64" title="Rock" width="50" height="50">
<img class="icon" id="paper" src="https://png.icons8.com/paper-filled/ios7/50" title="Paper Filled" width="50" height="50">
<img class="icon" id="scissors" src="https://png.icons8.com/scissors-filled/ios7/50" title="Scissors Filled" width="50" height="50">
<div class="wrapper">
<p id="output"></p>
</div>
<div style="border: 0px solid blue; display: block;">
<p id="myChoice"></p>
<p>- VS -</p>
<p id="cpuChoice"></p>
<hr>
<p id="verdict"></p>
</div>
<div id="winner">ðÂÂÂ</div>
<div id="loser">ð«</div>
<div id="tied">ðÂÂÂ</div>
I made this game for my SON, and he loves it BTW :)
javascript rock-paper-scissors
I'm learning javascript and want to make my code as efficient as possible. This is something I wrote a while back and I keep revising it as I learn new things. But since I'm partly OCD, I can't help but want to stop repeating myself (DRY).
Can someone please help me use Vanilla JS to make this cleaner?
Here is the final code at CodePen!
jsFiddle
// Start All the Variables //
var userChoice;
var watson;
var saying = "You chose " + userChoice + ". Watson chose " + watson;
var win = " You Win!";
var lose = " You Lose";
var tie = " We Tied";
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
// USER'S CHOICE
// *************
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
// WATSON'S CHOICE
// ***************
function checkWatson()
// generates a random number between 0-2
randomNum = Math.floor(Math.random() * 3);
// generate a random number and assign it to one of the 3 choices
if (randomNum === 0)
watson = "rock";
else if (randomNum === 1)
watson = "paper";
else
watson = "scissors";
console.log('Watson chose: ' + watson);
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
// COMPARE USER VS WATSON
// **********************
function compare()
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
cpuChoice.innerHTML = "Watson chose: " + watson;
<img class="icon" id="rock" src="https://png.icons8.com/rock/win8/64" title="Rock" width="50" height="50">
<img class="icon" id="paper" src="https://png.icons8.com/paper-filled/ios7/50" title="Paper Filled" width="50" height="50">
<img class="icon" id="scissors" src="https://png.icons8.com/scissors-filled/ios7/50" title="Scissors Filled" width="50" height="50">
<div class="wrapper">
<p id="output"></p>
</div>
<div style="border: 0px solid blue; display: block;">
<p id="myChoice"></p>
<p>- VS -</p>
<p id="cpuChoice"></p>
<hr>
<p id="verdict"></p>
</div>
<div id="winner">ðÂÂÂ</div>
<div id="loser">ð«</div>
<div id="tied">ðÂÂÂ</div>
I made this game for my SON, and he loves it BTW :)
// Start All the Variables //
var userChoice;
var watson;
var saying = "You chose " + userChoice + ". Watson chose " + watson;
var win = " You Win!";
var lose = " You Lose";
var tie = " We Tied";
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
// USER'S CHOICE
// *************
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
// WATSON'S CHOICE
// ***************
function checkWatson()
// generates a random number between 0-2
randomNum = Math.floor(Math.random() * 3);
// generate a random number and assign it to one of the 3 choices
if (randomNum === 0)
watson = "rock";
else if (randomNum === 1)
watson = "paper";
else
watson = "scissors";
console.log('Watson chose: ' + watson);
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
// COMPARE USER VS WATSON
// **********************
function compare()
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
cpuChoice.innerHTML = "Watson chose: " + watson;
<img class="icon" id="rock" src="https://png.icons8.com/rock/win8/64" title="Rock" width="50" height="50">
<img class="icon" id="paper" src="https://png.icons8.com/paper-filled/ios7/50" title="Paper Filled" width="50" height="50">
<img class="icon" id="scissors" src="https://png.icons8.com/scissors-filled/ios7/50" title="Scissors Filled" width="50" height="50">
<div class="wrapper">
<p id="output"></p>
</div>
<div style="border: 0px solid blue; display: block;">
<p id="myChoice"></p>
<p>- VS -</p>
<p id="cpuChoice"></p>
<hr>
<p id="verdict"></p>
</div>
<div id="winner">ðÂÂÂ</div>
<div id="loser">ð«</div>
<div id="tied">ðÂÂÂ</div>
// Start All the Variables //
var userChoice;
var watson;
var saying = "You chose " + userChoice + ". Watson chose " + watson;
var win = " You Win!";
var lose = " You Lose";
var tie = " We Tied";
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
// USER'S CHOICE
// *************
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
// WATSON'S CHOICE
// ***************
function checkWatson()
// generates a random number between 0-2
randomNum = Math.floor(Math.random() * 3);
// generate a random number and assign it to one of the 3 choices
if (randomNum === 0)
watson = "rock";
else if (randomNum === 1)
watson = "paper";
else
watson = "scissors";
console.log('Watson chose: ' + watson);
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
// COMPARE USER VS WATSON
// **********************
function compare()
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
cpuChoice.innerHTML = "Watson chose: " + watson;
<img class="icon" id="rock" src="https://png.icons8.com/rock/win8/64" title="Rock" width="50" height="50">
<img class="icon" id="paper" src="https://png.icons8.com/paper-filled/ios7/50" title="Paper Filled" width="50" height="50">
<img class="icon" id="scissors" src="https://png.icons8.com/scissors-filled/ios7/50" title="Scissors Filled" width="50" height="50">
<div class="wrapper">
<p id="output"></p>
</div>
<div style="border: 0px solid blue; display: block;">
<p id="myChoice"></p>
<p>- VS -</p>
<p id="cpuChoice"></p>
<hr>
<p id="verdict"></p>
</div>
<div id="winner">ðÂÂÂ</div>
<div id="loser">ð«</div>
<div id="tied">ðÂÂÂ</div>
javascript rock-paper-scissors
edited Jan 27 at 1:27
Martin York
70.9k481244
70.9k481244
asked Jan 27 at 0:28
user2117484
162
162
add a comment |Â
add a comment |Â
2 Answers
2
active
oldest
votes
up vote
2
down vote
Drying out the code.
You don't need OCD to want DRY code. It is a fundamental requirement of any good code (with a few special case exceptions) that it is DRY.
Looking at the code and first impression is that its "OMG bottom of the pool soaking wet."
How to dry code.
Look for repeating code. Yep obvious I know but look at the following...
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
Even the comments repeat, there are 12 var
,3 new audio
and http://facebook.design/public/sounds
, and 9 document.getElementById(
I removed only the relevant content and counted the characters. ~80% of the the above code is redundant. with only 12 unique values in there, the filenames of the audio, and the element ids, which you repeat again in the rest of the code, and HTML.
I don't think you know, but What makes it even more not dry is that every modern browser already has all the variables names created for you. Each line var winner = document.getElementById('winner');
is simply replacing the existing global variable. Removing all the get element lines and your code would run just as well.
Functions
If you write many functions with just a few minor differences its a good sign that you can write one function and pass the differences as arguments.
Each function has just two differences, and abstractly just one "The choice"
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
Each of the following also have just one abstract difference the result.
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
Each group of three can easily be replaced with one. The click functions can use the click event to get the needed information from the DOM, and the result functions can be passed an object that contain the sound, message and element to display the message.
Logic
Look for self similar logic. The following function has but 3 outcomes, with only two input values. but each input is referenced at least 3 times and at worst 7 times each. Even after you have displayed a result you still test for results that you already know are not true.
function compare() {
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
Data types
I don't mean to repeat my self with the obvious. BUT..
Dont repeat across data types. You have names in HTML and names in Javascript, though not always possible you can often use the data from one to provide the needed info for another. For example the id of the user clickable images "rock, paper, scissors" contain the info you need to name the hands.
DRY but not a desert.
It is not possible to be without repeating code, sometimes the complexity of the problem makes it hard to avoid repetition, or repetition may be the quickest way to solve the problem when time is short. Sometimes when execution performance is the driving force repetition is the only way to get the best performance.
The length some coders go to too avoid repeating code can become so complex and difficult to follow that they have lost any advantage they would get from DRY code. A DRY KISS is best.
DRY is practical, saves you time entering and modifying code. DRY code is more compact and thus should be easier to read and understand. If this does not happen then it was best left as it was.
Example
Your code was not bad, and the changes I would have made if DRY was not the considering option would have been just the win logic and remove the repeated click and end game functions.
Your code after a little drying out.
var userChoice, watson;
// array of strings in order [name of play, sound file, play result message]
const setup = [
["winner", "Success 3", " You Win!"],
["loser", "Error 1", " You Lose"],
["tied", "Collapse", " We Tied"]
];
// Set up the 3 results types
const plays = ;
setup.forEach(play =>
plays[play[0]] =
audio : new Audio(`http://facebook.design/public/sounds/$play[1].mp3`),
message : play[2],
element : document.querySelector("#"+play[0]),
;
);
// Handles the player hand selection.
buttons.addEventListener("click", event => 0];
cpuChoice.textContent = "Watson chose: " + watson;
compare();
);
const results = // names contain player choice then computer choice.
rock : // play picked rock
paper : plays.loser, // computer paper
scissors : plays.winner, // computer scissor
, // no need for draw condition.
paper :
rock : plays.winner,
scissors : plays.loser,
,
scissors :
rock : plays.loser,
paper : plays.winner,
function displayResults(play) // show results as supplied in play
if(!mute) play.audio.play() // added mute option.
verdict.textContent = play.message;
[winner,loser,tied].forEach(el => el.style.display = "none" );
play.element.style.display = "block";
function compare() // checks for draw or looks up result
if (watson === userChoice)
displayResults(plays.tied);
else
displayResults(results[userChoice][watson]);
//Added code to mute sound
var mute = false;
sound.addEventListener("click",event =>
mute = !mute;
sound.textContent = mute ? "ðÂÂÂ" : "ðÂÂÂ";
);
.icon
cursor: pointer;
.resultEmo
font-size : 28px;
<div id="buttons">
<img class="icon" id="rock" src="https://png.icons8.com/rock/win8/64" title="Rock" width="50" height="50">
<img class="icon" id="paper" src="https://png.icons8.com/paper-filled/ios7/50" title="Paper Filled" width="50" height="50">
<img class="icon" id="scissors" src="https://png.icons8.com/scissors-filled/ios7/50" title="Scissors Filled" width="50" height="50">
</div><span class="icon" id="sound" title="Click to toggle audio on and off">ðÂÂÂ</span>
<div class="wrapper">
<p id="output"></p>
</div>
<div style="border: 0px solid blue; display: block;">
<p id="myChoice"></p>
- VS -
<p id="cpuChoice"></p>
<hr>
<p id="verdict"></p>
</div>
<div class="resultEmo" id="winner" style ="display:none;">ðÂÂÂ</div>
<div class="resultEmo" id="loser" style ="display:none;">ð«</div>
<div class="resultEmo" id="tied" style ="display:none;">ðÂÂÂ</div>
ðÂÂÂWARNING the above snippet contains sound.
wow, thank you so much for the deep explanation. Fundamentally I knew that most the code was the same and I just needed to pass 1 or 2 things through to deliver a different outcome, but was unsure how. I really appreciate the time you spent on this. Now I understand it much better. Time to dissect your solution.
â user2117484
Jan 28 at 19:33
Why did you usedocument.querySelector("#"+play[0])
instead ofdocument.getElementById(play[0])
?getElementById
is faster thanquerySelector
. But as you said yourself, they are already stored in a global variable, so you could just usewindow[play[0]]
.
â Kruga
Jan 29 at 10:31
@Kruga Yes usingwindow[play[0]]
ordocument.getElementById
are alternatives. There is no particular reason I usedquerySelector
force of habit mostly.
â Blindman67
Jan 29 at 12:24
add a comment |Â
up vote
1
down vote
Your code looks quite nice and is easy to understand. It can be written much shorter though.
// Start All the Variables //
var userChoice;
var watson;
var saying = "You chose " + userChoice + ". Watson chose " + watson;
Oops, the last line contains a bug. The saying
variable is initialized here once and for all. It doesn't magically change whenever the userChoice
or the watson
changes.
To change this, you should define saying
as a function. When you call a function, its code is run each time. So it should look like this:
function saying()
return "You chose " + userChoice + ". Watson chose " + watson;
Continuing â¦
var win = " You Win!";
var lose = " You Lose";
var tie = " We Tied";
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
The code looks nice, but you can improve on the variable names. The first three names describe the outcome of a single game, therefore they should use the same grammatical form. I suggest win
, loss
, tie
.
The other names are fine. Instead of the comments at the end of the lines, you could rename rock
to rockButton
and so on. Then the comments become redundant.
// USER'S CHOICE
// *************
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
This code repeats three times. The only changes are the name of the chosen item and its choice number. To make this simpler, you can define these two things in the HTML as data
attributes. It looks like this:
<img class="icon"
id="scissors"
src="https://png.icons8.com/scissors-filled/ios7/50"
title="Scissors Filled"
width="50"
height="50"
data-choice="2"
data-name="Scissors">
Then, the JavaScript function can be changed to this:
function onButtonClick()
userChoice = this.getAttribute('data-choice');
myChoice.innerHTML = "You chose: " + this.getAttribute('data-name');
checkWatson();
compare();
And after that, assigning the events is trivial:
rock.addEventListener("click", onButtonClick);
paper.addEventListener("click", onButtonClick);
scissors.addEventListener("click", onButtonClick);
That's all. All the data that is needed for distinguishing the buttons is fetched in the onButtonClick
function.
// WATSON'S CHOICE
// ***************
function checkWatson()
// generates a random number between 0-2
randomNum = Math.floor(Math.random() * 3);
// generate a random number and assign it to one of the 3 choices
if (randomNum === 0)
watson = "rock";
else if (randomNum === 1)
watson = "paper";
else
watson = "scissors";
console.log('Watson chose: ' + watson);
Your code can be made a lot simpler if you keep the choices as numbers internally, instead of making them strings. There should be a single function that converts a number into a string. It can be written like this:
function choiceToName(choice)
return ["Rock", "Paper", "Scissors"][choice];
By calling console.log('Watson chose: ', choiceToName(randomNum))
you save one variable and several lines of code.
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
Again, these functions look very similar. They can be replaced with a single function that displays the outcome:
function displayOutcome()
var outcome = (3 + userChoice - cpuChoice) % 3;
// 0 = tie, 1 = loss, 2 = win
var audio = [audioTied, audioLoser, audioWinner][outcome];
audio.play();
var element = [tied, loser, winner][outcome];
verdict.innerHTML = element;
winner .style.display = 'hide';
loser .style.display = 'hide';
tied .style.display = 'hide';
element.style.display = 'block';
Continuing with your code:
// COMPARE USER VS WATSON
// **********************
function compare()
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
cpuChoice.innerHTML = "Watson chose: " + watson;
I have already reduced the decision of the winner into a single line of code in the first part of the displayOutcome
. Therefore the whole compare
function is not necessary anymore.
add a comment |Â
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
2
down vote
Drying out the code.
You don't need OCD to want DRY code. It is a fundamental requirement of any good code (with a few special case exceptions) that it is DRY.
Looking at the code and first impression is that its "OMG bottom of the pool soaking wet."
How to dry code.
Look for repeating code. Yep obvious I know but look at the following...
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
Even the comments repeat, there are 12 var
,3 new audio
and http://facebook.design/public/sounds
, and 9 document.getElementById(
I removed only the relevant content and counted the characters. ~80% of the the above code is redundant. with only 12 unique values in there, the filenames of the audio, and the element ids, which you repeat again in the rest of the code, and HTML.
I don't think you know, but What makes it even more not dry is that every modern browser already has all the variables names created for you. Each line var winner = document.getElementById('winner');
is simply replacing the existing global variable. Removing all the get element lines and your code would run just as well.
Functions
If you write many functions with just a few minor differences its a good sign that you can write one function and pass the differences as arguments.
Each function has just two differences, and abstractly just one "The choice"
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
Each of the following also have just one abstract difference the result.
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
Each group of three can easily be replaced with one. The click functions can use the click event to get the needed information from the DOM, and the result functions can be passed an object that contain the sound, message and element to display the message.
Logic
Look for self similar logic. The following function has but 3 outcomes, with only two input values. but each input is referenced at least 3 times and at worst 7 times each. Even after you have displayed a result you still test for results that you already know are not true.
function compare() {
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
Data types
I don't mean to repeat my self with the obvious. BUT..
Dont repeat across data types. You have names in HTML and names in Javascript, though not always possible you can often use the data from one to provide the needed info for another. For example the id of the user clickable images "rock, paper, scissors" contain the info you need to name the hands.
DRY but not a desert.
It is not possible to be without repeating code, sometimes the complexity of the problem makes it hard to avoid repetition, or repetition may be the quickest way to solve the problem when time is short. Sometimes when execution performance is the driving force repetition is the only way to get the best performance.
The length some coders go to too avoid repeating code can become so complex and difficult to follow that they have lost any advantage they would get from DRY code. A DRY KISS is best.
DRY is practical, saves you time entering and modifying code. DRY code is more compact and thus should be easier to read and understand. If this does not happen then it was best left as it was.
Example
Your code was not bad, and the changes I would have made if DRY was not the considering option would have been just the win logic and remove the repeated click and end game functions.
Your code after a little drying out.
var userChoice, watson;
// array of strings in order [name of play, sound file, play result message]
const setup = [
["winner", "Success 3", " You Win!"],
["loser", "Error 1", " You Lose"],
["tied", "Collapse", " We Tied"]
];
// Set up the 3 results types
const plays = ;
setup.forEach(play =>
plays[play[0]] =
audio : new Audio(`http://facebook.design/public/sounds/$play[1].mp3`),
message : play[2],
element : document.querySelector("#"+play[0]),
;
);
// Handles the player hand selection.
buttons.addEventListener("click", event => 0];
cpuChoice.textContent = "Watson chose: " + watson;
compare();
);
const results = // names contain player choice then computer choice.
rock : // play picked rock
paper : plays.loser, // computer paper
scissors : plays.winner, // computer scissor
, // no need for draw condition.
paper :
rock : plays.winner,
scissors : plays.loser,
,
scissors :
rock : plays.loser,
paper : plays.winner,
function displayResults(play) // show results as supplied in play
if(!mute) play.audio.play() // added mute option.
verdict.textContent = play.message;
[winner,loser,tied].forEach(el => el.style.display = "none" );
play.element.style.display = "block";
function compare() // checks for draw or looks up result
if (watson === userChoice)
displayResults(plays.tied);
else
displayResults(results[userChoice][watson]);
//Added code to mute sound
var mute = false;
sound.addEventListener("click",event =>
mute = !mute;
sound.textContent = mute ? "ðÂÂÂ" : "ðÂÂÂ";
);
.icon
cursor: pointer;
.resultEmo
font-size : 28px;
<div id="buttons">
<img class="icon" id="rock" src="https://png.icons8.com/rock/win8/64" title="Rock" width="50" height="50">
<img class="icon" id="paper" src="https://png.icons8.com/paper-filled/ios7/50" title="Paper Filled" width="50" height="50">
<img class="icon" id="scissors" src="https://png.icons8.com/scissors-filled/ios7/50" title="Scissors Filled" width="50" height="50">
</div><span class="icon" id="sound" title="Click to toggle audio on and off">ðÂÂÂ</span>
<div class="wrapper">
<p id="output"></p>
</div>
<div style="border: 0px solid blue; display: block;">
<p id="myChoice"></p>
- VS -
<p id="cpuChoice"></p>
<hr>
<p id="verdict"></p>
</div>
<div class="resultEmo" id="winner" style ="display:none;">ðÂÂÂ</div>
<div class="resultEmo" id="loser" style ="display:none;">ð«</div>
<div class="resultEmo" id="tied" style ="display:none;">ðÂÂÂ</div>
ðÂÂÂWARNING the above snippet contains sound.
wow, thank you so much for the deep explanation. Fundamentally I knew that most the code was the same and I just needed to pass 1 or 2 things through to deliver a different outcome, but was unsure how. I really appreciate the time you spent on this. Now I understand it much better. Time to dissect your solution.
â user2117484
Jan 28 at 19:33
Why did you usedocument.querySelector("#"+play[0])
instead ofdocument.getElementById(play[0])
?getElementById
is faster thanquerySelector
. But as you said yourself, they are already stored in a global variable, so you could just usewindow[play[0]]
.
â Kruga
Jan 29 at 10:31
@Kruga Yes usingwindow[play[0]]
ordocument.getElementById
are alternatives. There is no particular reason I usedquerySelector
force of habit mostly.
â Blindman67
Jan 29 at 12:24
add a comment |Â
up vote
2
down vote
Drying out the code.
You don't need OCD to want DRY code. It is a fundamental requirement of any good code (with a few special case exceptions) that it is DRY.
Looking at the code and first impression is that its "OMG bottom of the pool soaking wet."
How to dry code.
Look for repeating code. Yep obvious I know but look at the following...
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
Even the comments repeat, there are 12 var
,3 new audio
and http://facebook.design/public/sounds
, and 9 document.getElementById(
I removed only the relevant content and counted the characters. ~80% of the the above code is redundant. with only 12 unique values in there, the filenames of the audio, and the element ids, which you repeat again in the rest of the code, and HTML.
I don't think you know, but What makes it even more not dry is that every modern browser already has all the variables names created for you. Each line var winner = document.getElementById('winner');
is simply replacing the existing global variable. Removing all the get element lines and your code would run just as well.
Functions
If you write many functions with just a few minor differences its a good sign that you can write one function and pass the differences as arguments.
Each function has just two differences, and abstractly just one "The choice"
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
Each of the following also have just one abstract difference the result.
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
Each group of three can easily be replaced with one. The click functions can use the click event to get the needed information from the DOM, and the result functions can be passed an object that contain the sound, message and element to display the message.
Logic
Look for self similar logic. The following function has but 3 outcomes, with only two input values. but each input is referenced at least 3 times and at worst 7 times each. Even after you have displayed a result you still test for results that you already know are not true.
function compare() {
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
Data types
I don't mean to repeat my self with the obvious. BUT..
Dont repeat across data types. You have names in HTML and names in Javascript, though not always possible you can often use the data from one to provide the needed info for another. For example the id of the user clickable images "rock, paper, scissors" contain the info you need to name the hands.
DRY but not a desert.
It is not possible to be without repeating code, sometimes the complexity of the problem makes it hard to avoid repetition, or repetition may be the quickest way to solve the problem when time is short. Sometimes when execution performance is the driving force repetition is the only way to get the best performance.
The length some coders go to too avoid repeating code can become so complex and difficult to follow that they have lost any advantage they would get from DRY code. A DRY KISS is best.
DRY is practical, saves you time entering and modifying code. DRY code is more compact and thus should be easier to read and understand. If this does not happen then it was best left as it was.
Example
Your code was not bad, and the changes I would have made if DRY was not the considering option would have been just the win logic and remove the repeated click and end game functions.
Your code after a little drying out.
var userChoice, watson;
// array of strings in order [name of play, sound file, play result message]
const setup = [
["winner", "Success 3", " You Win!"],
["loser", "Error 1", " You Lose"],
["tied", "Collapse", " We Tied"]
];
// Set up the 3 results types
const plays = ;
setup.forEach(play =>
plays[play[0]] =
audio : new Audio(`http://facebook.design/public/sounds/$play[1].mp3`),
message : play[2],
element : document.querySelector("#"+play[0]),
;
);
// Handles the player hand selection.
buttons.addEventListener("click", event => 0];
cpuChoice.textContent = "Watson chose: " + watson;
compare();
);
const results = // names contain player choice then computer choice.
rock : // play picked rock
paper : plays.loser, // computer paper
scissors : plays.winner, // computer scissor
, // no need for draw condition.
paper :
rock : plays.winner,
scissors : plays.loser,
,
scissors :
rock : plays.loser,
paper : plays.winner,
function displayResults(play) // show results as supplied in play
if(!mute) play.audio.play() // added mute option.
verdict.textContent = play.message;
[winner,loser,tied].forEach(el => el.style.display = "none" );
play.element.style.display = "block";
function compare() // checks for draw or looks up result
if (watson === userChoice)
displayResults(plays.tied);
else
displayResults(results[userChoice][watson]);
//Added code to mute sound
var mute = false;
sound.addEventListener("click",event =>
mute = !mute;
sound.textContent = mute ? "ðÂÂÂ" : "ðÂÂÂ";
);
.icon
cursor: pointer;
.resultEmo
font-size : 28px;
<div id="buttons">
<img class="icon" id="rock" src="https://png.icons8.com/rock/win8/64" title="Rock" width="50" height="50">
<img class="icon" id="paper" src="https://png.icons8.com/paper-filled/ios7/50" title="Paper Filled" width="50" height="50">
<img class="icon" id="scissors" src="https://png.icons8.com/scissors-filled/ios7/50" title="Scissors Filled" width="50" height="50">
</div><span class="icon" id="sound" title="Click to toggle audio on and off">ðÂÂÂ</span>
<div class="wrapper">
<p id="output"></p>
</div>
<div style="border: 0px solid blue; display: block;">
<p id="myChoice"></p>
- VS -
<p id="cpuChoice"></p>
<hr>
<p id="verdict"></p>
</div>
<div class="resultEmo" id="winner" style ="display:none;">ðÂÂÂ</div>
<div class="resultEmo" id="loser" style ="display:none;">ð«</div>
<div class="resultEmo" id="tied" style ="display:none;">ðÂÂÂ</div>
ðÂÂÂWARNING the above snippet contains sound.
wow, thank you so much for the deep explanation. Fundamentally I knew that most the code was the same and I just needed to pass 1 or 2 things through to deliver a different outcome, but was unsure how. I really appreciate the time you spent on this. Now I understand it much better. Time to dissect your solution.
â user2117484
Jan 28 at 19:33
Why did you usedocument.querySelector("#"+play[0])
instead ofdocument.getElementById(play[0])
?getElementById
is faster thanquerySelector
. But as you said yourself, they are already stored in a global variable, so you could just usewindow[play[0]]
.
â Kruga
Jan 29 at 10:31
@Kruga Yes usingwindow[play[0]]
ordocument.getElementById
are alternatives. There is no particular reason I usedquerySelector
force of habit mostly.
â Blindman67
Jan 29 at 12:24
add a comment |Â
up vote
2
down vote
up vote
2
down vote
Drying out the code.
You don't need OCD to want DRY code. It is a fundamental requirement of any good code (with a few special case exceptions) that it is DRY.
Looking at the code and first impression is that its "OMG bottom of the pool soaking wet."
How to dry code.
Look for repeating code. Yep obvious I know but look at the following...
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
Even the comments repeat, there are 12 var
,3 new audio
and http://facebook.design/public/sounds
, and 9 document.getElementById(
I removed only the relevant content and counted the characters. ~80% of the the above code is redundant. with only 12 unique values in there, the filenames of the audio, and the element ids, which you repeat again in the rest of the code, and HTML.
I don't think you know, but What makes it even more not dry is that every modern browser already has all the variables names created for you. Each line var winner = document.getElementById('winner');
is simply replacing the existing global variable. Removing all the get element lines and your code would run just as well.
Functions
If you write many functions with just a few minor differences its a good sign that you can write one function and pass the differences as arguments.
Each function has just two differences, and abstractly just one "The choice"
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
Each of the following also have just one abstract difference the result.
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
Each group of three can easily be replaced with one. The click functions can use the click event to get the needed information from the DOM, and the result functions can be passed an object that contain the sound, message and element to display the message.
Logic
Look for self similar logic. The following function has but 3 outcomes, with only two input values. but each input is referenced at least 3 times and at worst 7 times each. Even after you have displayed a result you still test for results that you already know are not true.
function compare() {
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
Data types
I don't mean to repeat my self with the obvious. BUT..
Dont repeat across data types. You have names in HTML and names in Javascript, though not always possible you can often use the data from one to provide the needed info for another. For example the id of the user clickable images "rock, paper, scissors" contain the info you need to name the hands.
DRY but not a desert.
It is not possible to be without repeating code, sometimes the complexity of the problem makes it hard to avoid repetition, or repetition may be the quickest way to solve the problem when time is short. Sometimes when execution performance is the driving force repetition is the only way to get the best performance.
The length some coders go to too avoid repeating code can become so complex and difficult to follow that they have lost any advantage they would get from DRY code. A DRY KISS is best.
DRY is practical, saves you time entering and modifying code. DRY code is more compact and thus should be easier to read and understand. If this does not happen then it was best left as it was.
Example
Your code was not bad, and the changes I would have made if DRY was not the considering option would have been just the win logic and remove the repeated click and end game functions.
Your code after a little drying out.
var userChoice, watson;
// array of strings in order [name of play, sound file, play result message]
const setup = [
["winner", "Success 3", " You Win!"],
["loser", "Error 1", " You Lose"],
["tied", "Collapse", " We Tied"]
];
// Set up the 3 results types
const plays = ;
setup.forEach(play =>
plays[play[0]] =
audio : new Audio(`http://facebook.design/public/sounds/$play[1].mp3`),
message : play[2],
element : document.querySelector("#"+play[0]),
;
);
// Handles the player hand selection.
buttons.addEventListener("click", event => 0];
cpuChoice.textContent = "Watson chose: " + watson;
compare();
);
const results = // names contain player choice then computer choice.
rock : // play picked rock
paper : plays.loser, // computer paper
scissors : plays.winner, // computer scissor
, // no need for draw condition.
paper :
rock : plays.winner,
scissors : plays.loser,
,
scissors :
rock : plays.loser,
paper : plays.winner,
function displayResults(play) // show results as supplied in play
if(!mute) play.audio.play() // added mute option.
verdict.textContent = play.message;
[winner,loser,tied].forEach(el => el.style.display = "none" );
play.element.style.display = "block";
function compare() // checks for draw or looks up result
if (watson === userChoice)
displayResults(plays.tied);
else
displayResults(results[userChoice][watson]);
//Added code to mute sound
var mute = false;
sound.addEventListener("click",event =>
mute = !mute;
sound.textContent = mute ? "ðÂÂÂ" : "ðÂÂÂ";
);
.icon
cursor: pointer;
.resultEmo
font-size : 28px;
<div id="buttons">
<img class="icon" id="rock" src="https://png.icons8.com/rock/win8/64" title="Rock" width="50" height="50">
<img class="icon" id="paper" src="https://png.icons8.com/paper-filled/ios7/50" title="Paper Filled" width="50" height="50">
<img class="icon" id="scissors" src="https://png.icons8.com/scissors-filled/ios7/50" title="Scissors Filled" width="50" height="50">
</div><span class="icon" id="sound" title="Click to toggle audio on and off">ðÂÂÂ</span>
<div class="wrapper">
<p id="output"></p>
</div>
<div style="border: 0px solid blue; display: block;">
<p id="myChoice"></p>
- VS -
<p id="cpuChoice"></p>
<hr>
<p id="verdict"></p>
</div>
<div class="resultEmo" id="winner" style ="display:none;">ðÂÂÂ</div>
<div class="resultEmo" id="loser" style ="display:none;">ð«</div>
<div class="resultEmo" id="tied" style ="display:none;">ðÂÂÂ</div>
ðÂÂÂWARNING the above snippet contains sound.
Drying out the code.
You don't need OCD to want DRY code. It is a fundamental requirement of any good code (with a few special case exceptions) that it is DRY.
Looking at the code and first impression is that its "OMG bottom of the pool soaking wet."
How to dry code.
Look for repeating code. Yep obvious I know but look at the following...
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
Even the comments repeat, there are 12 var
,3 new audio
and http://facebook.design/public/sounds
, and 9 document.getElementById(
I removed only the relevant content and counted the characters. ~80% of the the above code is redundant. with only 12 unique values in there, the filenames of the audio, and the element ids, which you repeat again in the rest of the code, and HTML.
I don't think you know, but What makes it even more not dry is that every modern browser already has all the variables names created for you. Each line var winner = document.getElementById('winner');
is simply replacing the existing global variable. Removing all the get element lines and your code would run just as well.
Functions
If you write many functions with just a few minor differences its a good sign that you can write one function and pass the differences as arguments.
Each function has just two differences, and abstractly just one "The choice"
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
Each of the following also have just one abstract difference the result.
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
Each group of three can easily be replaced with one. The click functions can use the click event to get the needed information from the DOM, and the result functions can be passed an object that contain the sound, message and element to display the message.
Logic
Look for self similar logic. The following function has but 3 outcomes, with only two input values. but each input is referenced at least 3 times and at worst 7 times each. Even after you have displayed a result you still test for results that you already know are not true.
function compare() {
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
Data types
I don't mean to repeat my self with the obvious. BUT..
Dont repeat across data types. You have names in HTML and names in Javascript, though not always possible you can often use the data from one to provide the needed info for another. For example the id of the user clickable images "rock, paper, scissors" contain the info you need to name the hands.
DRY but not a desert.
It is not possible to be without repeating code, sometimes the complexity of the problem makes it hard to avoid repetition, or repetition may be the quickest way to solve the problem when time is short. Sometimes when execution performance is the driving force repetition is the only way to get the best performance.
The length some coders go to too avoid repeating code can become so complex and difficult to follow that they have lost any advantage they would get from DRY code. A DRY KISS is best.
DRY is practical, saves you time entering and modifying code. DRY code is more compact and thus should be easier to read and understand. If this does not happen then it was best left as it was.
Example
Your code was not bad, and the changes I would have made if DRY was not the considering option would have been just the win logic and remove the repeated click and end game functions.
Your code after a little drying out.
var userChoice, watson;
// array of strings in order [name of play, sound file, play result message]
const setup = [
["winner", "Success 3", " You Win!"],
["loser", "Error 1", " You Lose"],
["tied", "Collapse", " We Tied"]
];
// Set up the 3 results types
const plays = ;
setup.forEach(play =>
plays[play[0]] =
audio : new Audio(`http://facebook.design/public/sounds/$play[1].mp3`),
message : play[2],
element : document.querySelector("#"+play[0]),
;
);
// Handles the player hand selection.
buttons.addEventListener("click", event => 0];
cpuChoice.textContent = "Watson chose: " + watson;
compare();
);
const results = // names contain player choice then computer choice.
rock : // play picked rock
paper : plays.loser, // computer paper
scissors : plays.winner, // computer scissor
, // no need for draw condition.
paper :
rock : plays.winner,
scissors : plays.loser,
,
scissors :
rock : plays.loser,
paper : plays.winner,
function displayResults(play) // show results as supplied in play
if(!mute) play.audio.play() // added mute option.
verdict.textContent = play.message;
[winner,loser,tied].forEach(el => el.style.display = "none" );
play.element.style.display = "block";
function compare() // checks for draw or looks up result
if (watson === userChoice)
displayResults(plays.tied);
else
displayResults(results[userChoice][watson]);
//Added code to mute sound
var mute = false;
sound.addEventListener("click",event =>
mute = !mute;
sound.textContent = mute ? "ðÂÂÂ" : "ðÂÂÂ";
);
.icon
cursor: pointer;
.resultEmo
font-size : 28px;
<div id="buttons">
<img class="icon" id="rock" src="https://png.icons8.com/rock/win8/64" title="Rock" width="50" height="50">
<img class="icon" id="paper" src="https://png.icons8.com/paper-filled/ios7/50" title="Paper Filled" width="50" height="50">
<img class="icon" id="scissors" src="https://png.icons8.com/scissors-filled/ios7/50" title="Scissors Filled" width="50" height="50">
</div><span class="icon" id="sound" title="Click to toggle audio on and off">ðÂÂÂ</span>
<div class="wrapper">
<p id="output"></p>
</div>
<div style="border: 0px solid blue; display: block;">
<p id="myChoice"></p>
- VS -
<p id="cpuChoice"></p>
<hr>
<p id="verdict"></p>
</div>
<div class="resultEmo" id="winner" style ="display:none;">ðÂÂÂ</div>
<div class="resultEmo" id="loser" style ="display:none;">ð«</div>
<div class="resultEmo" id="tied" style ="display:none;">ðÂÂÂ</div>
ðÂÂÂWARNING the above snippet contains sound.
var userChoice, watson;
// array of strings in order [name of play, sound file, play result message]
const setup = [
["winner", "Success 3", " You Win!"],
["loser", "Error 1", " You Lose"],
["tied", "Collapse", " We Tied"]
];
// Set up the 3 results types
const plays = ;
setup.forEach(play =>
plays[play[0]] =
audio : new Audio(`http://facebook.design/public/sounds/$play[1].mp3`),
message : play[2],
element : document.querySelector("#"+play[0]),
;
);
// Handles the player hand selection.
buttons.addEventListener("click", event => 0];
cpuChoice.textContent = "Watson chose: " + watson;
compare();
);
const results = // names contain player choice then computer choice.
rock : // play picked rock
paper : plays.loser, // computer paper
scissors : plays.winner, // computer scissor
, // no need for draw condition.
paper :
rock : plays.winner,
scissors : plays.loser,
,
scissors :
rock : plays.loser,
paper : plays.winner,
function displayResults(play) // show results as supplied in play
if(!mute) play.audio.play() // added mute option.
verdict.textContent = play.message;
[winner,loser,tied].forEach(el => el.style.display = "none" );
play.element.style.display = "block";
function compare() // checks for draw or looks up result
if (watson === userChoice)
displayResults(plays.tied);
else
displayResults(results[userChoice][watson]);
//Added code to mute sound
var mute = false;
sound.addEventListener("click",event =>
mute = !mute;
sound.textContent = mute ? "ðÂÂÂ" : "ðÂÂÂ";
);
.icon
cursor: pointer;
.resultEmo
font-size : 28px;
<div id="buttons">
<img class="icon" id="rock" src="https://png.icons8.com/rock/win8/64" title="Rock" width="50" height="50">
<img class="icon" id="paper" src="https://png.icons8.com/paper-filled/ios7/50" title="Paper Filled" width="50" height="50">
<img class="icon" id="scissors" src="https://png.icons8.com/scissors-filled/ios7/50" title="Scissors Filled" width="50" height="50">
</div><span class="icon" id="sound" title="Click to toggle audio on and off">ðÂÂÂ</span>
<div class="wrapper">
<p id="output"></p>
</div>
<div style="border: 0px solid blue; display: block;">
<p id="myChoice"></p>
- VS -
<p id="cpuChoice"></p>
<hr>
<p id="verdict"></p>
</div>
<div class="resultEmo" id="winner" style ="display:none;">ðÂÂÂ</div>
<div class="resultEmo" id="loser" style ="display:none;">ð«</div>
<div class="resultEmo" id="tied" style ="display:none;">ðÂÂÂ</div>
var userChoice, watson;
// array of strings in order [name of play, sound file, play result message]
const setup = [
["winner", "Success 3", " You Win!"],
["loser", "Error 1", " You Lose"],
["tied", "Collapse", " We Tied"]
];
// Set up the 3 results types
const plays = ;
setup.forEach(play =>
plays[play[0]] =
audio : new Audio(`http://facebook.design/public/sounds/$play[1].mp3`),
message : play[2],
element : document.querySelector("#"+play[0]),
;
);
// Handles the player hand selection.
buttons.addEventListener("click", event => 0];
cpuChoice.textContent = "Watson chose: " + watson;
compare();
);
const results = // names contain player choice then computer choice.
rock : // play picked rock
paper : plays.loser, // computer paper
scissors : plays.winner, // computer scissor
, // no need for draw condition.
paper :
rock : plays.winner,
scissors : plays.loser,
,
scissors :
rock : plays.loser,
paper : plays.winner,
function displayResults(play) // show results as supplied in play
if(!mute) play.audio.play() // added mute option.
verdict.textContent = play.message;
[winner,loser,tied].forEach(el => el.style.display = "none" );
play.element.style.display = "block";
function compare() // checks for draw or looks up result
if (watson === userChoice)
displayResults(plays.tied);
else
displayResults(results[userChoice][watson]);
//Added code to mute sound
var mute = false;
sound.addEventListener("click",event =>
mute = !mute;
sound.textContent = mute ? "ðÂÂÂ" : "ðÂÂÂ";
);
.icon
cursor: pointer;
.resultEmo
font-size : 28px;
<div id="buttons">
<img class="icon" id="rock" src="https://png.icons8.com/rock/win8/64" title="Rock" width="50" height="50">
<img class="icon" id="paper" src="https://png.icons8.com/paper-filled/ios7/50" title="Paper Filled" width="50" height="50">
<img class="icon" id="scissors" src="https://png.icons8.com/scissors-filled/ios7/50" title="Scissors Filled" width="50" height="50">
</div><span class="icon" id="sound" title="Click to toggle audio on and off">ðÂÂÂ</span>
<div class="wrapper">
<p id="output"></p>
</div>
<div style="border: 0px solid blue; display: block;">
<p id="myChoice"></p>
- VS -
<p id="cpuChoice"></p>
<hr>
<p id="verdict"></p>
</div>
<div class="resultEmo" id="winner" style ="display:none;">ðÂÂÂ</div>
<div class="resultEmo" id="loser" style ="display:none;">ð«</div>
<div class="resultEmo" id="tied" style ="display:none;">ðÂÂÂ</div>
edited Jan 27 at 19:00
answered Jan 27 at 15:16
Blindman67
5,4001320
5,4001320
wow, thank you so much for the deep explanation. Fundamentally I knew that most the code was the same and I just needed to pass 1 or 2 things through to deliver a different outcome, but was unsure how. I really appreciate the time you spent on this. Now I understand it much better. Time to dissect your solution.
â user2117484
Jan 28 at 19:33
Why did you usedocument.querySelector("#"+play[0])
instead ofdocument.getElementById(play[0])
?getElementById
is faster thanquerySelector
. But as you said yourself, they are already stored in a global variable, so you could just usewindow[play[0]]
.
â Kruga
Jan 29 at 10:31
@Kruga Yes usingwindow[play[0]]
ordocument.getElementById
are alternatives. There is no particular reason I usedquerySelector
force of habit mostly.
â Blindman67
Jan 29 at 12:24
add a comment |Â
wow, thank you so much for the deep explanation. Fundamentally I knew that most the code was the same and I just needed to pass 1 or 2 things through to deliver a different outcome, but was unsure how. I really appreciate the time you spent on this. Now I understand it much better. Time to dissect your solution.
â user2117484
Jan 28 at 19:33
Why did you usedocument.querySelector("#"+play[0])
instead ofdocument.getElementById(play[0])
?getElementById
is faster thanquerySelector
. But as you said yourself, they are already stored in a global variable, so you could just usewindow[play[0]]
.
â Kruga
Jan 29 at 10:31
@Kruga Yes usingwindow[play[0]]
ordocument.getElementById
are alternatives. There is no particular reason I usedquerySelector
force of habit mostly.
â Blindman67
Jan 29 at 12:24
wow, thank you so much for the deep explanation. Fundamentally I knew that most the code was the same and I just needed to pass 1 or 2 things through to deliver a different outcome, but was unsure how. I really appreciate the time you spent on this. Now I understand it much better. Time to dissect your solution.
â user2117484
Jan 28 at 19:33
wow, thank you so much for the deep explanation. Fundamentally I knew that most the code was the same and I just needed to pass 1 or 2 things through to deliver a different outcome, but was unsure how. I really appreciate the time you spent on this. Now I understand it much better. Time to dissect your solution.
â user2117484
Jan 28 at 19:33
Why did you use
document.querySelector("#"+play[0])
instead of document.getElementById(play[0])
? getElementById
is faster than querySelector
. But as you said yourself, they are already stored in a global variable, so you could just use window[play[0]]
.â Kruga
Jan 29 at 10:31
Why did you use
document.querySelector("#"+play[0])
instead of document.getElementById(play[0])
? getElementById
is faster than querySelector
. But as you said yourself, they are already stored in a global variable, so you could just use window[play[0]]
.â Kruga
Jan 29 at 10:31
@Kruga Yes using
window[play[0]]
or document.getElementById
are alternatives. There is no particular reason I used querySelector
force of habit mostly.â Blindman67
Jan 29 at 12:24
@Kruga Yes using
window[play[0]]
or document.getElementById
are alternatives. There is no particular reason I used querySelector
force of habit mostly.â Blindman67
Jan 29 at 12:24
add a comment |Â
up vote
1
down vote
Your code looks quite nice and is easy to understand. It can be written much shorter though.
// Start All the Variables //
var userChoice;
var watson;
var saying = "You chose " + userChoice + ". Watson chose " + watson;
Oops, the last line contains a bug. The saying
variable is initialized here once and for all. It doesn't magically change whenever the userChoice
or the watson
changes.
To change this, you should define saying
as a function. When you call a function, its code is run each time. So it should look like this:
function saying()
return "You chose " + userChoice + ". Watson chose " + watson;
Continuing â¦
var win = " You Win!";
var lose = " You Lose";
var tie = " We Tied";
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
The code looks nice, but you can improve on the variable names. The first three names describe the outcome of a single game, therefore they should use the same grammatical form. I suggest win
, loss
, tie
.
The other names are fine. Instead of the comments at the end of the lines, you could rename rock
to rockButton
and so on. Then the comments become redundant.
// USER'S CHOICE
// *************
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
This code repeats three times. The only changes are the name of the chosen item and its choice number. To make this simpler, you can define these two things in the HTML as data
attributes. It looks like this:
<img class="icon"
id="scissors"
src="https://png.icons8.com/scissors-filled/ios7/50"
title="Scissors Filled"
width="50"
height="50"
data-choice="2"
data-name="Scissors">
Then, the JavaScript function can be changed to this:
function onButtonClick()
userChoice = this.getAttribute('data-choice');
myChoice.innerHTML = "You chose: " + this.getAttribute('data-name');
checkWatson();
compare();
And after that, assigning the events is trivial:
rock.addEventListener("click", onButtonClick);
paper.addEventListener("click", onButtonClick);
scissors.addEventListener("click", onButtonClick);
That's all. All the data that is needed for distinguishing the buttons is fetched in the onButtonClick
function.
// WATSON'S CHOICE
// ***************
function checkWatson()
// generates a random number between 0-2
randomNum = Math.floor(Math.random() * 3);
// generate a random number and assign it to one of the 3 choices
if (randomNum === 0)
watson = "rock";
else if (randomNum === 1)
watson = "paper";
else
watson = "scissors";
console.log('Watson chose: ' + watson);
Your code can be made a lot simpler if you keep the choices as numbers internally, instead of making them strings. There should be a single function that converts a number into a string. It can be written like this:
function choiceToName(choice)
return ["Rock", "Paper", "Scissors"][choice];
By calling console.log('Watson chose: ', choiceToName(randomNum))
you save one variable and several lines of code.
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
Again, these functions look very similar. They can be replaced with a single function that displays the outcome:
function displayOutcome()
var outcome = (3 + userChoice - cpuChoice) % 3;
// 0 = tie, 1 = loss, 2 = win
var audio = [audioTied, audioLoser, audioWinner][outcome];
audio.play();
var element = [tied, loser, winner][outcome];
verdict.innerHTML = element;
winner .style.display = 'hide';
loser .style.display = 'hide';
tied .style.display = 'hide';
element.style.display = 'block';
Continuing with your code:
// COMPARE USER VS WATSON
// **********************
function compare()
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
cpuChoice.innerHTML = "Watson chose: " + watson;
I have already reduced the decision of the winner into a single line of code in the first part of the displayOutcome
. Therefore the whole compare
function is not necessary anymore.
add a comment |Â
up vote
1
down vote
Your code looks quite nice and is easy to understand. It can be written much shorter though.
// Start All the Variables //
var userChoice;
var watson;
var saying = "You chose " + userChoice + ". Watson chose " + watson;
Oops, the last line contains a bug. The saying
variable is initialized here once and for all. It doesn't magically change whenever the userChoice
or the watson
changes.
To change this, you should define saying
as a function. When you call a function, its code is run each time. So it should look like this:
function saying()
return "You chose " + userChoice + ". Watson chose " + watson;
Continuing â¦
var win = " You Win!";
var lose = " You Lose";
var tie = " We Tied";
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
The code looks nice, but you can improve on the variable names. The first three names describe the outcome of a single game, therefore they should use the same grammatical form. I suggest win
, loss
, tie
.
The other names are fine. Instead of the comments at the end of the lines, you could rename rock
to rockButton
and so on. Then the comments become redundant.
// USER'S CHOICE
// *************
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
This code repeats three times. The only changes are the name of the chosen item and its choice number. To make this simpler, you can define these two things in the HTML as data
attributes. It looks like this:
<img class="icon"
id="scissors"
src="https://png.icons8.com/scissors-filled/ios7/50"
title="Scissors Filled"
width="50"
height="50"
data-choice="2"
data-name="Scissors">
Then, the JavaScript function can be changed to this:
function onButtonClick()
userChoice = this.getAttribute('data-choice');
myChoice.innerHTML = "You chose: " + this.getAttribute('data-name');
checkWatson();
compare();
And after that, assigning the events is trivial:
rock.addEventListener("click", onButtonClick);
paper.addEventListener("click", onButtonClick);
scissors.addEventListener("click", onButtonClick);
That's all. All the data that is needed for distinguishing the buttons is fetched in the onButtonClick
function.
// WATSON'S CHOICE
// ***************
function checkWatson()
// generates a random number between 0-2
randomNum = Math.floor(Math.random() * 3);
// generate a random number and assign it to one of the 3 choices
if (randomNum === 0)
watson = "rock";
else if (randomNum === 1)
watson = "paper";
else
watson = "scissors";
console.log('Watson chose: ' + watson);
Your code can be made a lot simpler if you keep the choices as numbers internally, instead of making them strings. There should be a single function that converts a number into a string. It can be written like this:
function choiceToName(choice)
return ["Rock", "Paper", "Scissors"][choice];
By calling console.log('Watson chose: ', choiceToName(randomNum))
you save one variable and several lines of code.
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
Again, these functions look very similar. They can be replaced with a single function that displays the outcome:
function displayOutcome()
var outcome = (3 + userChoice - cpuChoice) % 3;
// 0 = tie, 1 = loss, 2 = win
var audio = [audioTied, audioLoser, audioWinner][outcome];
audio.play();
var element = [tied, loser, winner][outcome];
verdict.innerHTML = element;
winner .style.display = 'hide';
loser .style.display = 'hide';
tied .style.display = 'hide';
element.style.display = 'block';
Continuing with your code:
// COMPARE USER VS WATSON
// **********************
function compare()
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
cpuChoice.innerHTML = "Watson chose: " + watson;
I have already reduced the decision of the winner into a single line of code in the first part of the displayOutcome
. Therefore the whole compare
function is not necessary anymore.
add a comment |Â
up vote
1
down vote
up vote
1
down vote
Your code looks quite nice and is easy to understand. It can be written much shorter though.
// Start All the Variables //
var userChoice;
var watson;
var saying = "You chose " + userChoice + ". Watson chose " + watson;
Oops, the last line contains a bug. The saying
variable is initialized here once and for all. It doesn't magically change whenever the userChoice
or the watson
changes.
To change this, you should define saying
as a function. When you call a function, its code is run each time. So it should look like this:
function saying()
return "You chose " + userChoice + ". Watson chose " + watson;
Continuing â¦
var win = " You Win!";
var lose = " You Lose";
var tie = " We Tied";
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
The code looks nice, but you can improve on the variable names. The first three names describe the outcome of a single game, therefore they should use the same grammatical form. I suggest win
, loss
, tie
.
The other names are fine. Instead of the comments at the end of the lines, you could rename rock
to rockButton
and so on. Then the comments become redundant.
// USER'S CHOICE
// *************
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
This code repeats three times. The only changes are the name of the chosen item and its choice number. To make this simpler, you can define these two things in the HTML as data
attributes. It looks like this:
<img class="icon"
id="scissors"
src="https://png.icons8.com/scissors-filled/ios7/50"
title="Scissors Filled"
width="50"
height="50"
data-choice="2"
data-name="Scissors">
Then, the JavaScript function can be changed to this:
function onButtonClick()
userChoice = this.getAttribute('data-choice');
myChoice.innerHTML = "You chose: " + this.getAttribute('data-name');
checkWatson();
compare();
And after that, assigning the events is trivial:
rock.addEventListener("click", onButtonClick);
paper.addEventListener("click", onButtonClick);
scissors.addEventListener("click", onButtonClick);
That's all. All the data that is needed for distinguishing the buttons is fetched in the onButtonClick
function.
// WATSON'S CHOICE
// ***************
function checkWatson()
// generates a random number between 0-2
randomNum = Math.floor(Math.random() * 3);
// generate a random number and assign it to one of the 3 choices
if (randomNum === 0)
watson = "rock";
else if (randomNum === 1)
watson = "paper";
else
watson = "scissors";
console.log('Watson chose: ' + watson);
Your code can be made a lot simpler if you keep the choices as numbers internally, instead of making them strings. There should be a single function that converts a number into a string. It can be written like this:
function choiceToName(choice)
return ["Rock", "Paper", "Scissors"][choice];
By calling console.log('Watson chose: ', choiceToName(randomNum))
you save one variable and several lines of code.
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
Again, these functions look very similar. They can be replaced with a single function that displays the outcome:
function displayOutcome()
var outcome = (3 + userChoice - cpuChoice) % 3;
// 0 = tie, 1 = loss, 2 = win
var audio = [audioTied, audioLoser, audioWinner][outcome];
audio.play();
var element = [tied, loser, winner][outcome];
verdict.innerHTML = element;
winner .style.display = 'hide';
loser .style.display = 'hide';
tied .style.display = 'hide';
element.style.display = 'block';
Continuing with your code:
// COMPARE USER VS WATSON
// **********************
function compare()
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
cpuChoice.innerHTML = "Watson chose: " + watson;
I have already reduced the decision of the winner into a single line of code in the first part of the displayOutcome
. Therefore the whole compare
function is not necessary anymore.
Your code looks quite nice and is easy to understand. It can be written much shorter though.
// Start All the Variables //
var userChoice;
var watson;
var saying = "You chose " + userChoice + ". Watson chose " + watson;
Oops, the last line contains a bug. The saying
variable is initialized here once and for all. It doesn't magically change whenever the userChoice
or the watson
changes.
To change this, you should define saying
as a function. When you call a function, its code is run each time. So it should look like this:
function saying()
return "You chose " + userChoice + ". Watson chose " + watson;
Continuing â¦
var win = " You Win!";
var lose = " You Lose";
var tie = " We Tied";
var audioWinner = new Audio('http://facebook.design/public/sounds/Success 3.mp3');
var audioLoser = new Audio('http://facebook.design/public/sounds/Error 1.mp3');
var audioTied = new Audio('http://facebook.design/public/sounds/Collapse.mp3');
var winner = document.getElementById('winner');
var loser = document.getElementById('loser');
var tied = document.getElementById('tied');
var rock = document.getElementById("rock"); // rock button el
var paper = document.getElementById("paper"); // paper button el
var scissors = document.getElementById("scissors"); // scissors button el
var myChoice = document.getElementById("myChoice"); // user choice el
var cpuChoice = document.getElementById("cpuChoice"); // watson choice el
var verdict = document.getElementById("verdict"); // verdict el
The code looks nice, but you can improve on the variable names. The first three names describe the outcome of a single game, therefore they should use the same grammatical form. I suggest win
, loss
, tie
.
The other names are fine. Instead of the comments at the end of the lines, you could rename rock
to rockButton
and so on. Then the comments become redundant.
// USER'S CHOICE
// *************
rock.addEventListener("click", function()
userChoice = 0;
myChoice.innerHTML = "You chose: Rock";
checkWatson();
compare();
);
paper.addEventListener("click", function()
userChoice = 1;
myChoice.innerHTML = "You chose: Paper";
checkWatson();
compare();
);
scissors.addEventListener("click", function()
userChoice = 2;
myChoice.innerHTML = "You chose: Scissors";
checkWatson();
compare();
);
This code repeats three times. The only changes are the name of the chosen item and its choice number. To make this simpler, you can define these two things in the HTML as data
attributes. It looks like this:
<img class="icon"
id="scissors"
src="https://png.icons8.com/scissors-filled/ios7/50"
title="Scissors Filled"
width="50"
height="50"
data-choice="2"
data-name="Scissors">
Then, the JavaScript function can be changed to this:
function onButtonClick()
userChoice = this.getAttribute('data-choice');
myChoice.innerHTML = "You chose: " + this.getAttribute('data-name');
checkWatson();
compare();
And after that, assigning the events is trivial:
rock.addEventListener("click", onButtonClick);
paper.addEventListener("click", onButtonClick);
scissors.addEventListener("click", onButtonClick);
That's all. All the data that is needed for distinguishing the buttons is fetched in the onButtonClick
function.
// WATSON'S CHOICE
// ***************
function checkWatson()
// generates a random number between 0-2
randomNum = Math.floor(Math.random() * 3);
// generate a random number and assign it to one of the 3 choices
if (randomNum === 0)
watson = "rock";
else if (randomNum === 1)
watson = "paper";
else
watson = "scissors";
console.log('Watson chose: ' + watson);
Your code can be made a lot simpler if you keep the choices as numbers internally, instead of making them strings. There should be a single function that converts a number into a string. It can be written like this:
function choiceToName(choice)
return ["Rock", "Paper", "Scissors"][choice];
By calling console.log('Watson chose: ', choiceToName(randomNum))
you save one variable and several lines of code.
// 3 OUTCOME FUNCTIONS
// *******************
function resultsTie()
audioTied.play();
verdict.innerHTML = tie; // tie
winner.style.display = 'none';
loser.style.display = 'none';
tied.style.display = 'block';
function resultsWinner()
audioWinner.play();
verdict.innerHTML = win; // win
winner.style.display = 'block';
loser.style.display = 'none';
tied.style.display = 'none';
function resultsLoser()
audioLoser.play();
verdict.innerHTML = lose;
winner.style.display = 'none';
loser.style.display = 'block';
tied.style.display = 'none';
Again, these functions look very similar. They can be replaced with a single function that displays the outcome:
function displayOutcome()
var outcome = (3 + userChoice - cpuChoice) % 3;
// 0 = tie, 1 = loss, 2 = win
var audio = [audioTied, audioLoser, audioWinner][outcome];
audio.play();
var element = [tied, loser, winner][outcome];
verdict.innerHTML = element;
winner .style.display = 'hide';
loser .style.display = 'hide';
tied .style.display = 'hide';
element.style.display = 'block';
Continuing with your code:
// COMPARE USER VS WATSON
// **********************
function compare()
// user chooses rock
if (userChoice === randomNum)
resultsTie();
else if (userChoice === 0 && randomNum === 1)
resultsLoser();
else if (userChoice === 0 && randomNum === 2)
resultsWinner();
// user chooses paper
if (userChoice === 1 && randomNum === 0)
resultsWinner();
else if (userChoice === 1 && randomNum === 2)
resultsLoser();
// user chooses scissors
if (userChoice === 2 && randomNum === 0)
resultsLoser();
else if (userChoice === 2 && randomNum === 1)
resultsWinner();
cpuChoice.innerHTML = "Watson chose: " + watson;
I have already reduced the decision of the winner into a single line of code in the first part of the displayOutcome
. Therefore the whole compare
function is not necessary anymore.
answered Jan 27 at 15:21
Roland Illig
10.4k11543
10.4k11543
add a comment |Â
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f186103%2frock-paper-scissors-in-javascript%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password