Simple number theory game, pt. 3

Question

More refactoring based you guys' great feedback. This time around I worked on using global/local variables properly and efficiently, putting return values to use, renaming variables so that the code is more understandable, and simplifying functions so that they serve one main purpose.

My main concerns/focuses now: I'm not sure that I like the global variable newScoreAlertIndex and how it's being implemented, and am working on how to better write the newScoreAlert function. Also, since the while loop can essentially be divided up into two structurally equivalent parts (the user's turn and the computer's turn), I was debating creating a function to run a full turn, and call it twice in the while loop, switching between user and computer factor/score parameters. However I felt like this blurred the line between query and command, so I left it as is. Let me know what you think.

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 compScore = 2,
    userScore = 2,
    operator,
    newScoreAlertIndex = 1;

function getFactors(integer) {
    var factors = [];
    for(var i = 1; i <= integer; i++){
        if(integer % i === 0){
            factors.push(i);
        }
    }
    return factors;
}

function getUserFactor() {
    var possibleFactors = getFactors(compScore);
    var factor;
    while (true) {
        factor = parseInt(prompt("Your score: " + userScore + ". Computer's score: " + compScore +  ". Which factor do you choose from the computer's score?"));
        if (possibleFactors.indexOf(factor) !== -1) {
            if (userScore !== 2 ||
            userScore === 2 && compScore === 3 ||
            userScore === 2 && compScore !== 3 && factor !== 1 && factor !== 3) {
                return factor;
            }
        }
        alert("Not a valid entry.");
    }
}

function getCompFactor() {
    var possibleFactors = getFactors(userScore);
    var factor = possibleFactors[Math.floor(Math.random() * possibleFactors.length)];
    if (compScore === 2 && userScore !== 3) {
        while (factor === 1 || factor === 3) {
            factor = possibleFactors[Math.floor(Math.random() * possibleFactors.length)];
        }
    }
    return factor;
}

function updateScore(chosenFactor, oldScore) {
    var newScore;
    if (chosenFactor % 2 === 0) {
        newScore = oldScore + chosenFactor;
        operator = "added to";
    } else {
        newScore = oldScore - chosenFactor;
        if (newScore < 0) {
            newScore = newScore * -1;
        }
        operator = "subtracted from";
    }
    return newScore;
}

function newScoreAlert(chosenFactor, operator, newScore) {
    var alertText;
    if (newScoreAlertIndex % 2 !== 0) { alertText = " your score. You now have "; }
    else { alertText = " the computer's score. The computer now has "; }
    alert(chosenFactor + " was " + operator + alertText + newScore + ".");
    newScoreAlertIndex += 1;
}

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


while(true) {

    var userFactor = getUserFactor();
    userScore = updateScore(userFactor, userScore);
    newScoreAlert(userFactor, operator, userScore);
    if (checkForWin(userScore)) {
        alert("YAY! You won! :D");
        break;
    }

    var compFactor = getCompFactor();
    compScore = updateScore(compFactor, compScore);
    newScoreAlert(compFactor, operator, compScore);
    if (checkForWin(compScore)) {
        alert("Sorry! You lost :/");
        break;
    }
}

Answer 1

A few suggestions to make your code more readable/succinct:

1. Why not store your alerts in a single array?

var alerts = [alert("....",
              alert("....",...`

This makes your code more maintainable and your variable names less arbitrary. We have to agree rules2 is not such a hot name for a variable.

Note I don't know if the syntax I have above is correct. You might need to use bind or apply or something so the alert doesn't execute right away, but you can easily check this with a fast experiment...

Note in comments below SirPython has clarified that storing them this way will immediately run them. Consider his suggestion of storing the messages in an array rather than an alert function itself.

2. getFactors does a lot of unnecessary checks

It's well known that to get the factors of a number, you only need to check numbers less than its square root. That's, by definition, the largest factor a number can have other than itself. So you can adjust the for loop like so:

check_lim = Math.floor(sqrt(integer)) + 1   
for(var i = 1; i< check_lim; i++) { 
   if(integer % i == 0){
       factors.push(i);
       factors.push(integer/i);

Relatedly, I'd also suggest changing integer to something that sounds less like a key/reserved word. Makes me nervous!

Also, now your factors won't be in order from smallest to greatest. If an ordered factor list is necessary for your code, and if you really are just sticking with small numbers, your present method could be better.

3. Keep control statements as simple as possible

I'd rewrite checkForWin like so:

function checkForWin(number) {
    return ( number === 1  || number === 0 );
}

Answer 2

You've gotten rid of a lot of global variables, but you can do even better!

Consider writing this function:

function playGame(player, computer) {
   # start playing the game with initial scores: player, computer
   while (true) {
     var userFactor = getUserFactor(computer);
     ...
     var compFactor = getCompFactor(player);
     ...
   }
}

The main difference is that getUserFactor(...) and getCompFactor(...) now take an argument instead of referencing global variables.

Also, do the same with newScoreAlertIndex...

function playGame(player, computer) {
  var newScoreAlertIndex = 0;
  ...
  newScoreAlert(userFactor, operator, userScore, newScoreAlert);
  newScoreAlertIndex++;
  ...
}

The changes are:

1. Make newScoreAlertIndex a local variable.
2. Pass it into newScoreAlert.
3. Modify it in the main loop outside of newScoreAlert.

In fact, you should be able to find a way to eliminate the need for this variable - during the player's turn it is always odd and during the computer's turn it is always even.

Answer 3

A few things that you can improve on:

---

alert:

• There's no need to declare alerts as variables as:
  1. You don't use them.
  2. They return nothing.

• Don't have long lines of text, seperate the sentences into lines, like the following:

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!");

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.");

alert("A few exceptions:" + "\n\n"
    + "When 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\n"
    + "Neither 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\n"
    + "Exception to the exception! "
    + "In the case that you have a 2 and the computer has a 3, it is allowable to choose either factor.");

Notice that I seperated out the new-lines as well, that helps for readability also.

---

Data Structure:

Consider turning your data structure of defined variables into a data object, like so:

var data = {
      computer_score: 2
    , user_score: 2
    , operator
    , new_score_alert_index: 1
}

---

getUserFactor:

• You can simplify the following:

var possibleFactors = getFactors(compScore);
var factor;

into:

var possibleFactors = getFactors(compScore),
    factor;

• The long parseInt & alert call should be spread over multiple lines, like the alerts mentioned before:

    factor = parseInt(prompt("Your score: " + userScore + ". Computer's score: " + compScore +  ". Which factor do you choose from the computer's score?"));

    factor = parseInt(
                prompt("Your score: "
                    + userScore
                    + ". Computer's score: "
                    + compScore
                    + ". Which factor do you choose from the computer's score?"
                    )
             );

• You should move the or operator in the if statement to the beginning of the new-lines:

if (userScore !== 2
    || userScore === 2 && compScore === 3
    || userScore === 2 && compScore !== 3 && factor !== 1 && factor !== 3) {
    return factor;
}

---

getCompFactor:

• As you're testing whether factor is one of \$n\$ options, you can just make an array with all \$n\$ options, and ask whether factor is in it:

while (factor === 1 || factor === 3) {

into:

while (factor in [1, 3]) {

---

UpdateScore:

• You do not need to multiply newScore by negative one to return the negative value, just return the negative value:

newScore = newScore * -1;

into:

newScore = -newScore;

---

newScoreAlert:

• Instead of if-else blocks to assign alertText, use a ternary:

var alertText;
if (newScoreAlertIndex % 2 !== 0) { alertText = " your score. You now have "; }
else { alertText = " the computer's score. The computer now has "; }

into:

var alertText = (newScoreAlertIndex % 2 !== 0
    ? " your score. You now have "
    : " the computer's score. The computer now has ");

• You should spread the alert call out over a few lines:

alert(chosenFactor+ " was " + operator + alertText + newScore + ".");

into:

alert(chosenFactor
    + " was "
    + operator
    + alertText
    + newScore
    + ".");

• You don't need to += 1 or -= 1, you can use ++ and -- respectively:

newScoreAlertIndex +1;

into:

newScoreAlertIndex++;

---

checkForWin:

• You can heavily simplify this function, because you can return a boolean condition, so, the following could be simplified:

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

into:

function checkForWin(number) {
    return (number in [0, 1]);
}

Notice the in operator testing whether the number is in the array.

Answer 4

Lots of good answers already. I just want to look at one thing in particular, namely the way the computer chooses a factor.

You have these lines:

if (compScore === 2 && userScore !== 3) {
    while (factor === 1 || factor === 3) {
        factor = possibleFactors[Math.floor(Math.random() * possibleFactors.length)];
    }
}

My issue is with the while loop. This is really just a theoretical concern, but there's a (astronomically unlikely) possibility that the loop runs forever - or at least, runs for a loooong time - because it randomly keeps picking 1 or 3 over and over again. Or it picks a winner first time out, and it's not a problem. But the point is that you don't know.

Again, it's not a practical concern, but in terms of abstract "correctness" it's a concern.

I'd recommend simply removing those two factors from the list, so they won't be picked to begin with. Of course, this requires more code, but it does make the whole thing more correct.

A simple filter implementation would be something like:

var allowedFactors = factors.filter(function (factor) {
  return factor !== 1 && factor !== 3;
});

Of course it's still only relevant if the computer's score is 2 and the user's score isn't 3.

You can make it more generic, in case your rules change again, and you have to blacklist arbitrary factors:

function filterFactors(factors, excluded) {
  return factors.filter(function (factor) {
    return excluded.indexOf(factor) === -1;
  });
}

// usage:
var allowed = filterFactors(factors, [1, 3]);