11
\$\begingroup\$

I have been learning AngularJS and have written my first app. It's a Hangman AI which learns words and guesses the solution based on the words it knows.

Please look over my code and provide some pointers towards better practices, techniques and overall criticism.

Here is a CodePen demo, as the Stack Snippet may have cross-site AJAX issues.

var app = angular.module("HangManAI", []);

app.controller('HangManController', ['$scope', '$http', function($scope, $http){
	$scope.step = 0;
	$scope.theWords 		= new Array();
	$scope.wordCount 		= 0;
	$scope.wordLetters 		= new Array();
	$scope.myGuess 			= "None";
	$scope.testString		= "";
	$scope.attempts			=10;
	$scope.letterFound		= false;
	$scope.myMouth			= "G'Day. I am a hangman AI written with AngularJS. I can win a game of hangman against you if I know the words you use. Click New Game to begin.";
	$scope.error			= "";
	
	var availableLetters 	= ["A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z"];
	var knownWords = [];
	
	//Functions
	$scope.startAGame = function(){
		$scope.theWords = new Array();
		$scope.wordCount = 0;
		$scope.attempts = 10;
		$scope.error = "";
		availableLetters 	= ["A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z"];
		$scope.myMouth = "Alright! First tell me how many words there are and how many letters for each word.";
		$scope.step = 1;
		var req = {
				method: 'GET',
				url: 'http://www.hott-source.com/hangman/getMemory.php',
				data: { }
			}
			$http(req).then(function(response){
				knownWords = response.data;
				$scope.testString = knownWords;
				
			}, function(response){
				$scope.error = "I could not get my memory. Try reloading the page.";
			});
	}
	$scope.checkWords = function(){
		//Loop through each typed word
		var i = $scope.theWords.length;
		var wordKnown = false;
		var newWords = "";
		var whatToSay = "";
		while(i--){
			var currentWord = $scope.theWords[i].word;
			//Check if we know it.
			if(typeof knownWords[currentWord.length-1] === 'undefined')
			{
				
			}
			else{
				var innerLoop = knownWords[currentWord.length-1].length;
				while(innerLoop--){
					if(knownWords[currentWord.length-1][innerLoop] == currentWord)
					{
						wordKnown = true;
					}
				}
			}
			if(!wordKnown)
			{
				if(newWords != "")
				{
					newWords += ":";
					whatToSay += ", ";
				}
				else
					whatToSay = "I didn't know these words; ";
				newWords += currentWord;
				whatToSay += currentWord;
			}
			$scope.step=5;
			$scope.error = whatToSay;
		}
		//This needs to save to memory
		
		var myUrl = 'http://www.hott-source.com/hangman/getMemory.php';
		if(newWords != "")
			myUrl += '?newWords='+newWords;
		
		var req = {
			method: 'GET',
			url: myUrl,
			data: { }
		}
		$scope.myMouth = "";
		$http(req).then(function(response){
			$scope.myMouth = "Do you want to play again?";
		}, function(response){
			$scope.myMouth = "I could not get my memory. Try reloading the page.";
		});
	}
	$scope.nextLetter = function()
	{
		if($scope.letterFound)
			$scope.letterFound = false;
		else
			$scope.attempts -=1;
		
		if($scope.attempts > 0)
			chooseALetter();
		else
		{
			$scope.myMouth = "Damn it... please show me the missing letters. Each click will change the letter. Click done when its right";
			$scope.step = 4;
		}
	}
	$scope.fixLetter = function(letterIndex, wordIndex)
	{
		var aWord = this.theWords[wordIndex].word;
		var aLetter = aWord[letterIndex];
		switch(aLetter)
		{
			case "A":
				aLetter = "B";
				break;
			case "B":
				aLetter = "C";
				break;
			case "C":
				aLetter = "D";
				break;
			case "D":
				aLetter = "E";
				break;
			case "E":
				aLetter = "F";
				break;
			case "F":
				aLetter = "G";
				break;
			case "G":
				aLetter = "H";
				break;
			case "H":
				aLetter = "I";
				break;
			case "I":
				aLetter = "J";
				break;
			case "J":
				aLetter = "K";
				break;
			case "K":
				aLetter = "L";
				break;
			case "L":
				aLetter = "M";
				break;
			case "M":
				aLetter = "N";
				break;
			case "N":
				aLetter = "O";
				break;
			case "O":
				aLetter = "P";
				break;
			case "P":
				aLetter = "Q";
				break;
			case "Q":
				aLetter = "R";
				break;
			case "R":
				aLetter = "S";
				break;
			case "S":
				aLetter = "T";
				break;
			case "T":
				aLetter = "U";
				break;
			case "U":
				aLetter = "V";
				break;
			case "V":
				aLetter = "W";
				break;
			case "W":
				aLetter = "X";
				break;
			case "X":
				aLetter = "Y";
				break;
			case "Y":
				aLetter = "Z";
				break;
			case "Z":
				aLetter = "A";
				break;
			default:
				aLetter = "A";
		}
		aWord = aWord.replaceAt(letterIndex, aLetter);
		this.theWords[wordIndex].word = aWord;
	}
	$scope.foundLetter = function(letterIndex, wordIndex)
	{
		$scope.letterFound = true;
		var aWord = this.theWords[wordIndex].word;
		aWord = aWord.replaceAt(letterIndex, this.myGuess);
		this.theWords[wordIndex].word = aWord;
		var hasNotFinished = false;
		for(var i = 0; i<this.wordCount; i++)
		{
			var theWord = this.theWords[i].word;
			var j = theWord.length;
			while(j--){
				if(theWord[j] == "_")
				{
					hasNotFinished = true;
					break;
				}
			}
			if(hasNotFinished)
				break;
		}
		//Make some way to reverse
		if(!hasNotFinished)
		{
			$scope.myMouth = "YAY!, I GOT IT";
			$scope.checkWords();
		}
	}
	$scope.createTempWords = function(){
		//If the word array is bigger than the number of words we want, slice it.
		if(this.theWords.length > this.wordCount)
			this.theWords = this.theWords.slice(0, -1);
		else
		{
			//Otherwise push in another word.
			for(var i = this.theWords.length; i<this.wordCount; i++)
			{
				this.theWords.push({id:i, word: "_", letters:1});
			}
		}
	}
	//Increase or decrease the size of the words
	$scope.wordSize = function(num){
		if(this.theWords[num].word.length > this.theWords[num].letters)
			this.theWords[num].word = this.theWords[num].word.substr(0, this.theWords[num].letters);
		else
		{
			//Otherwise push in another word.
			for(var i = this.theWords[num].word.length; i<this.theWords[num].letters; i++)
			{
				this.theWords[num].word += "_";
			}
		}
	}
	//Player has finished inputting their words.
	$scope.readyToPlay = function(){
		if($scope.theWords.length < 1)
			$scope.myMouth = "You need to enter some words first.";
		else{
			$scope.instructions = "Click the button which matches the letter location, or click no!";
			$scope.step = 2;
			chooseALetter();
		}
	}
	//Enables me to delete an element from an array based on value.
	 Array.prototype.deleteElem = function(val) {
		var index = this.indexOf(val); 
		if (index >= 0) this.splice(index, 1);
		return this;
	}
	//Enables me to replace a character at a set index of a string
	String.prototype.replaceAt=function(index, character) {
		return this.substr(0, index) + character + this.substr(index+character.length);
	}
	//Choose the most likely letter out of all known words with the lengths equal to the words in the phrase.
	function chooseALetter()
	{
		var letterCount = [];
		for(var i=0;i<availableLetters.length;i++){
			letterCount[i] = 0;
		}
		//Loop through each secret word
		for(var phraseWord_i = 0; phraseWord_i<$scope.theWords.length; phraseWord_i++)
		{
			var thisPhraseWord = $scope.theWords[phraseWord_i].word;
			var phraseWordLength = thisPhraseWord.length;
			//Loop through all the words of the same length
			if(typeof knownWords[phraseWordLength-1] === 'undefined')
			{
				
			}
			else
			{
				for(var knownWord_i = 0; knownWord_i<knownWords[phraseWordLength-1].length; knownWord_i++){
					var thisKnownWord = knownWords[phraseWordLength-1][knownWord_i];
					//
					var i = thisKnownWord.length;
					var canUse = true;
					//Check if this word is allowed to be used
					while (i--) {
						//Has this letter been discovered?
					  if(thisPhraseWord[i] != "_"){
						  //Does thisKnownWord match that letter
						  if(thisPhraseWord[i] != thisKnownWord[i]){
							  canUse = false
						  }
					  }
					}
					//If this word can use add 1 to all the letterCounts
					if(canUse){
						var i = thisKnownWord.length;
						while(i--){
							letterCount[availableLetters.indexOf(thisKnownWord[i])] += 1;
						}
					}
				}
			}
		}			
		
		//Find the most used letter
		var maxIndex = 0;
		var max = letterCount[0];
		
		for (var i = 1; i < letterCount.length; i++) {
			if (letterCount[i] > max) {
				maxIndex = i;
				max = letterCount[i];
			}
		}
		
		$scope.myGuess = availableLetters[maxIndex];
		$scope.myMouth = "Are there any " + $scope.myGuess + "'s in your phrase?";
		availableLetters.deleteElem(availableLetters[maxIndex]);
	}
}]);

app.filter('isPlural', function(){
	return function(input) {
	 if(input > 1 || input == 0)
		 return "s";
	 else
		 return "";
	}
});

app.filter('dashes', function(){
	return function(input) {
		var result = "";
	 for(var i =0; i<input.length; i++)
	 {
		 result += "_ ";
	 }
	 return result;
	}
});
.error{
  color: red;
}
.inLineList{
  list-style: none;
  float: left;
  padding-right: 20px;
}
p{
  clear:left;
}
<script src="https://ajax.googleapis.com/ajax/libs/angularjs/1.2.23/angular.min.js"></script>
<body ng-app="HangManAI">
  	<script src="http://ajax.googleapis.com/ajax/libs/angularjs/1.4.8/angular.min.js"></script>	
	<div ng-controller='HangManController'>
		<p>{{myMouth}}</p>
		<p class='error'>{{error}}</p>
		<div ng-show='step == 0'>
			<button ng-click='startAGame()'>New Game</button>
		</div>
		<div ng-show='step==1'>
			<input type='number' ng-model='wordCount' ng-change='createTempWords()' min=0>
			<p>You want {{wordCount}} word{{wordCount | isPlural}}</p>
			<ol>
				<li ng-repeat='aWord in theWords track by $index'><input type='number' ng-model='aWord.letters' ng-change='wordSize($index)' ng-show='!gameOn'><button ng-click='foundLetter($index, $parent.$index)' ng-disabled='!gameOn' ng-repeat='letter in aWord.word track by $index'>{{ letter }}</button> {{aWord.letters}} letter{{aWord.letters | isPlural}}</li>
			</ol>
			<button id='startGame' ng-click='readyToPlay()' ng-show='!gameOn'>Start The Game</button>
		</div>
		<div ng-show='step==2'>
			<p>I have {{attempts}} time{{attempts | isPlural}} I can guess wrong</p>
			<ol>
				<li class='inLineList' ng-repeat='aWord in theWords track by $index'><button ng-click='foundLetter($index, $parent.$index)' ng-repeat='letter in aWord.word track by $index'>{{ letter }}</button></li>
			</ol>
			<p><button ng-show='!letterFound' ng-click='nextLetter()'>No</button><button ng-show='letterFound' ng-click='nextLetter()'>Next Letter</button></p>
		</div>
		<div ng-show='step==4 || step==5'>
			<ol>
				<li ng-repeat='aWord in theWords track by $index'><button ng-click='fixLetter($index, $parent.$index)' ng-repeat='letter in aWord.word track by $index'>{{ letter }}</button></li>
			</ol>
			<p><button ng-show='step==4' ng-click='checkWords()'>Done</button></p>
			<p><button ng-show='step==5' ng-click='startAGame()'>New Game</button></p>
		</div>
	</div>
	
	<script src='js/modules/HangManAI.js'></script>
	<script src='js/controllers/HangManController.js'></script>
	<script src='js/filters/isPlural.js'></script>
	<!--<script src='js/services/getMemory.js'></script>-->
</body>
</html>

\$\endgroup\$
4
  • \$\begingroup\$ Welcome to Code Review! I assumed the off-site code matched the code you had in your post, and copied it into an on-site executable Stack Snippet. Feel free to rollback the edit if I assumed wrong, and/or to further edit your post so that the code matches exactly what's in your IDE (I believe you're using tabs instead of spaces here, which doesn't play very well with Markdown). \$\endgroup\$ Mar 3, 2016 at 4:33
  • 1
    \$\begingroup\$ @Mat'sMug The Stack Snippet didn't work for me — cross-site AJAX problems, I think. I've restored the CodePen link. \$\endgroup\$ Mar 3, 2016 at 5:41
  • \$\begingroup\$ Thanks bruno. Yes I do plan to add most used letters, and phrases. It already works out which letter is used the most within the known words. \$\endgroup\$
    – Tom Hanson
    Mar 3, 2016 at 9:58
  • \$\begingroup\$ You are not allowed to edit the code in a question according to tips received in an answer as they will invalidate the answer and remove the ability for this post to help future users. \$\endgroup\$
    – SirPython
    Mar 3, 2016 at 22:35

1 Answer 1

7
\$\begingroup\$

Here are a few general feedbacks:

Have a coherent code style

You're mixing multiple code styles

  • sometimes you put spaces around operators ng-show='step == 0', sometimes you don't ng-show='step==2'
  • sometimes you put braces on same line $scope.checkWords = function(){, sometimes you don't

    $scope.nextLetter = function()
    {
    

Coherent code style improves readability, some IDEs have reformatting tools to help with this issue.

Avoid extending Javascript's classes

Prefer a function

function deleteElem(array, val) {
    var index = array.indexOf(val);
    if (index >= 0) array.splice(index, 1);
    return array;
}

over extending Array.prototype

 Array.prototype.deleteElem = function(val) { /*...*/ }

Who knows what will happen in future releases of Javascript? That may break your code, or you may break another library's code.

Avoid writing initialization values at multiple places

["A", "B", "C", "D", "E", "F", "G", "H" ... is a big initialization value. If it contains an error, you don't want to have to fix it at multiple places. You can extract the value in a constant, and clone it to initialize the variable.

var ALL_AVAILABLE_LETTERS = ["A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z"];
var availableLetters = ALL_AVAILABLE_LETTERS.concat(); //clone the array

Learn about character code and character comparison

That will probably help you simplify your algorithms. For example, you can rewrite fixLetter like this:

$scope.fixLetter = function(letterIndex, wordIndex) {
    var aWord = this.theWords[wordIndex].word;
    var aLetter = aWord[letterIndex];
    if(aLetter >= 'A' && aLetter < 'Z') {
        aLetter = String.fromCharCode(aLetter.charCodeAt(0) + 1);
    } else if(aLetter === 'Z') {
        aLetter = 'A';
    } else { //default
        aLetter = 'A';
    }
    aWord = aWord.replaceAt(letterIndex, aLetter);
    this.theWords[wordIndex].word = aWord;
}

Some documentation can be found on MDN: charCodeAt, fromCharCode

Comment intended empty blocks

It seems like you have a bug or unfinished stuff there:

if(typeof knownWords[currentWord.length-1] === 'undefined')
{

}

If you intend it to be empty, you can inverse the condition to avoid the else:

if(!(typeof knownWords[currentWord.length-1] === 'undefined'))
{
    var innerLoop = ...
}

Or you can add a comment:

if(typeof knownWords[currentWord.length-1] === 'undefined')
{
    //Do nothing (on purpose)
}

If it's unfinished code, add a TODO

if(typeof knownWords[currentWord.length-1] === 'undefined')
{
    //TODO [Describe what you need to code here]
}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Thank you. This is great stuff and exactly why I put it on here. \$\endgroup\$
    – Tom Hanson
    Mar 3, 2016 at 8:43
  • \$\begingroup\$ Can you please explain this line aLetter = String.fromCharCode(aLetter.charCodeAt(0)); I understand it is getting the character code of the first character of aLetter, and then using fromCharCode to set a new character to aLetter. Does this line need aLetter = String.fromCharCode(aLetter.charCodeAt(0)+1); \$\endgroup\$
    – Tom Hanson
    Mar 3, 2016 at 8:51
  • \$\begingroup\$ Nevermind, just tested it. I needed the +1 in it. \$\endgroup\$
    – Tom Hanson
    Mar 3, 2016 at 8:56
  • 1
    \$\begingroup\$ @tom-hanson Indeed I forgot the +1, I fixed the answer \$\endgroup\$
    – Volune
    Mar 3, 2016 at 8:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.