7
\$\begingroup\$

After receiving some awesome feedback in my first post yesterday, I was able to grasp the "building block" method of creating javascript in chunks and putting them together to create coherent/straightforward code.

I've done quite a bit of refactoring and am fairly pleased given my initial goals for this project, but I know that there is always room for improvement! So here's the updated code for anyone interested in taking a look. The main thing that still bugs me now is the newNumber variable. Whereas everything else is nice and tucked away in its own function, newNumber gets passed around in several different places. Hopefully that makes sense!

I suppose I should note that the game mechanics are a little different this time around. I realized that the previous iteration involved less strategy than I originally intended, so I mixed some things up. I also focused less on making the computer think a couple steps ahead, so the game will play more randomly now.

var welcome = alert("Welcome to Zeros or Ones! Both you and the computer will start the game off with the number '2'. Whoever gets down to '1' or '0' first wins!"),

rules = alert("This is a turn based game. You must first choose a factor from the computer's number. If the chosen factor is even, it is added to your current score; if odd, it is subtracted. After this, the computer chooses a factor from your new number. This back-and-forth continues until someone gets their number down to 1 or 0."),

rules2 = alert("A few exceptions:\n\nWhen an odd number is being subtracted from your score, if your resulting score is a negative number, the absolute value is taken, turning the negative number into a positive.\n\nNeither player is allowed to win the game on a 2. For instance if your score is 2, you cannot choose the factors 1 or 3 from the computer's score.\n\nException to the exception! In the case that you have a 2 and the computer has a 3, it is allowable to choose either factor.");


var compNumber = 2,
	yourNumber = 2,
	newNumber,
	yourDiv,
	compDiv,
	operator,
	youWin = "YAY! You won! :D",
	compWins = "Sorry! You lost :/";

function getFactors(integer) {
	var factors = [],
	quotient = 0;
	for(var i = 1; i <= integer; i++){
    	quotient = integer/i;
    	if(quotient === Math.floor(quotient)){
      		factors.push(i); 
    	}
  	}
  	return factors;
}

function playerChoosesFactor() {
	var initCompFactors = getFactors(compNumber);
	while (true) {
		var yourDivv = prompt("Your score: " + yourNumber + ". Computer's score: " + compNumber +  ". Which factor do you choose from the computer's score?");
        yourDiv = parseInt(yourDivv);

		if (initCompFactors.indexOf(yourDiv) !== -1 && yourNumber === 2 && compNumber !== 3 && yourDiv !== 1 && yourDiv !== 3) {
			return yourDiv;
		}
		if (initCompFactors.indexOf(yourDiv) !== -1 && yourNumber === 2 && compNumber === 3) {
			return yourDiv;
		}
		if (initCompFactors.indexOf(yourDiv) !== -1 && yourNumber !== 2) {
			return yourDiv;
		}
		alert("Not a valid entry.");
	}
}

function computerChoosesFactor() {
	var yourFactors = getFactors(yourNumber);
	compDiv = yourFactors[Math.floor(Math.random() * yourFactors.length)];
	if (compNumber === 2 && yourNumber !== 3) {
		while (compDiv === 1 || compDiv === 3) {
			compDiv = yourFactors[Math.floor(Math.random() * yourFactors.length)];
		}
	}
}

function updateScore(div, whoseScore, oldNumber) {
	if (div % 2 === 0) {
    	newNumber = oldNumber + div;
    	operator = "added to";
    } else {
    	newNumber = oldNumber - div;
    	if (newNumber < 0) {
			newNumber = newNumber * -1;
		}
    	operator = "subtracted from";
    }
    alert(div + " was " + operator + whoseScore + newNumber + ".");
    return newNumber;
}

function checkForWin(number, winOrLoss) {
	if (number === 1 || number === 0) {
	    alert(winOrLoss);
	    return true;
	} else {
		return false;
	}
}


while(true) {

	playerChoosesFactor();
	updateScore(yourDiv, " your score. You now have ", yourNumber);
	yourNumber = newNumber;
	if (checkForWin(yourNumber, youWin)) {break;}

	computerChoosesFactor();
	updateScore(compDiv, " the computer's score. The computer now has ", compNumber);
	compNumber = newNumber;
	if (checkForWin(compNumber, compWins)) {break;}

}

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Any reason why you didn't incorporate @Flambino's getFactors()? \$\endgroup\$ – 200_success Sep 2 '15 at 5:50
  • 2
    \$\begingroup\$ Not an answer, but you indented way too much. 4 spaces is standard in many places, not 8. Also minor but I would line up the names in the var declarations \$\endgroup\$ – Caridorc Sep 2 '15 at 7:37
  • 1
    \$\begingroup\$ Adding to Caridorc's comment: It looks like there's a mix of spaces and tabs, which leads to some excessive and/or strange indentation. Make sure your editor is set up to use one or the other, and don't mix and match them yourself. \$\endgroup\$ – Flambino Sep 2 '15 at 11:23
  • 1
    \$\begingroup\$ Thanks for the responses. As for the indent thing - sorry about that, I recently started using a new text editor and have yet to get the indentation down - I guess I've been too excited to just work on the program! I will make sure it's not an issue from here on out. @200_success I knew I was forgetting something. I will update my code to use Flambino's shortened getFactors() function. Thanks for catching that :) \$\endgroup\$ – Chris Sep 2 '15 at 12:17
  • \$\begingroup\$ Follow-up question \$\endgroup\$ – 200_success Sep 2 '15 at 18:31
3
\$\begingroup\$

Nice update, kudos. Still, I have some comments.

  1. As mentioned in a comment: Check your indentation. As Caridorc says, 4 spaces of indentation (or 1 tab, rendered with a width of 4 spaces) is a common standard for JavaScript. I tend to use 2 spaces myself, but that's less prevalent. So check your editor's settings.

  2. Global variables
    You have a bunch of them - more than you need. And more than you want; generally you'll want to keep the number of global variables to a minimum.

    An example can be found in updateScore. It uses a global operator variable, yet that variable doesn't need to be global. Its value is set and used all within updateScore, and thus it should be a local variable.

  3. Return values
    This is related to the above, but I notice you're rarely using a function's return value for anything. Your function both alters a global variable, and returns that same value; pick one or the other.

For instance, you have these three lines:

playerChoosesFactor();
updateScore(yourDiv, " your score. You now have ", yourNumber);
yourNumber = newNumber;

Looking at just this, the logic isn't clear. That is, it makes sense as text ("these are the steps..."), but less so as actual code: yourDiv and youNumber are just there, somehow. It's not clear they're being changed. It's also a little odd that even though playerChoosesFactor actually returns something, that return value is just ignored. Some other languages/compilers will actually complain if you don't do something with a return value, since that's probably a bug.

Generally you'll want to treat your functions as either "commands" or "queries". This isn't a hard and fast rule or anything, but it helps keep things clear. A command changes something (i.e. a global score value) but doesn't (need to) return anything, while a query merely returns something, but doesn't cause things outside its scope to change (i.e. return a list of factors for a given number). So for instance, playerChoosesFactor makes the most sense as a query-type function. Simply getting the user's choice doesn't change anything - nor should it.

On a side note, the mix between "player" and "you" to designate the user is a little confusion. Not terribly so, but in the interest of consistency, I'd stick to one or the other - or pick "user", since that's the usual opposite of "computer" ("player" could mean either one). I'd also suggest calling the score variables something with score in it rather than just number, i.e. userScore instead of yourNumber. There's also a consistency thing with "factor" vs. "div"; player chooses a factor, yet it's called a divisor. Sure, same deal, but that's all the more reason to call it the same.

Anyway, something slightly more straight forward, with regard to return values, would be:

var factor = getUserFactor(); // factor is a local variable, since it's only used for these two lines
updateUserScore(userFactor); // a "command" function, no return

I've left out the text you use to create the alert, but that's just for clarity.

There is a point, though, to keeping the logic and the user interface - the alerts in this case - separate. Right now, you're coupling them tightly although there's no need. You can alert the user without updating the score, or update a score without alerting anyone. Yes, it makes sense to do both at the same time, but it's not intrinsically linked, so making them so in code is not really called for.

\$\endgroup\$
10
  • \$\begingroup\$ I've learned so much these past 2 days, I seriously can't thank you guys enough. I've read through everyone's feedback and have made more changes. Will post the new version soon. Thanks again :) \$\endgroup\$ – Chris Sep 2 '15 at 16:32
  • \$\begingroup\$ @Chris No problem, glad you found it all useful! Looking forward to see what you come up with \$\endgroup\$ – Flambino Sep 2 '15 at 16:34
  • \$\begingroup\$ And hopefully I'll actually get the indentation right this time ;) \$\endgroup\$ – Chris Sep 2 '15 at 16:35
  • \$\begingroup\$ @Chris Just a note to say I've updated my answer a little bit. I tweaked the last code block to use a updateUserScore function (which I'll just pretend exists), since the previous iteration wasn't really workable. It'd have to return something, since it wouldn't know which score to update. So it didn't make a good example of the points I'd made elsewhere in the answer :) \$\endgroup\$ – Flambino Sep 2 '15 at 17:00
  • 1
    \$\begingroup\$ @Caridorc Thanks. I think I'd heard of the idea years ago, but this talk on testing brought it into focus (great talk in general!). Here's the wikipedia article on the concept \$\endgroup\$ – Flambino Sep 2 '15 at 19:02
1
\$\begingroup\$

I suggest you adhere further to the single responsability principle:

 function checkForWin(number, winOrLoss) {
    if (number === 1 || number === 0) {
        alert(winOrLoss);
        return true;
    } else {
        return false;
        }
}

I would remove the extraneous alert as this is logic only function and simplify to

 return (number === 1 || number === 0);

You may alert after doing the check.


You always do the same thing after the ifs checks:

    if (initCompFactors.indexOf(yourDiv) !== -1 && yourNumber === 2 && compNumber !== 3 && yourDiv !== 1 && yourDiv !== 3) {
        return yourDiv;
    }
    if (initCompFactors.indexOf(yourDiv) !== -1 && yourNumber === 2 && compNumber === 3) {
        return yourDiv;
    }
    if (initCompFactors.indexOf(yourDiv) !== -1 && yourNumber !== 2) {
        return yourDiv;
    }

I suggest merging them into an or (||).


You need a helper function:

initCompFactors.indexOf(yourDiv) !== -1

This is not easy to understand for a Non-Javascripter, it can be as easy as:

 function isInside(item, list) {
      return list.indexOf(item) !== -1;
 }

You may use that instead.


Indenting should be 4 spaces deep, not 8. And you should use spaces only as @Flambino noted.


Avoid assigning None to a variable:

var welcome = alert(...)

alert returns nothing so you should remove the assignement.

\$\endgroup\$
8
  • \$\begingroup\$ About removing the alert from the checkForWin function, would you also suggest removing the alert in the updateScore function? \$\endgroup\$ – Chris Sep 2 '15 at 13:08
  • \$\begingroup\$ Also, concerning the repetition of checks that all return yourDiv - I actually considered it, but decided not to condense the 3 separate statements into 1. I felt that it would be quite a lengthy condition and would be more confusing to those first looking at the code. My next comment is my attempt at condensing the if checks, while also trying not to be too lengthy in the condition. \$\endgroup\$ – Chris Sep 2 '15 at 13:19
  • \$\begingroup\$ @Chris just format it on separate lines \$\endgroup\$ – Caridorc Sep 2 '15 at 13:21
  • \$\begingroup\$ if (initCompFactors.indexOf(yourDiv) !== -1) { if (yourNumber !== 2 || yourNumber === 2 && compNumber === 3 || yourNumber === 2 && compNumber !== 3 && yourDiv !== 1 && yourDiv !== 3) { return yourDiv; } } \$\endgroup\$ – Chris Sep 2 '15 at 13:21
  • 1
    \$\begingroup\$ Ohh, I see what you mean. Thanks for clarifying :) I feel silly for not thinking of that already \$\endgroup\$ – Chris Sep 2 '15 at 13:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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