4
\$\begingroup\$

I've made a simple game for testing math facts (think practice for elementary school-age kids). It displays a random addition, subtraction, multiplication, or division problem, and feedback on if the right answer was entered.

For now it works interacts with the user via prompt(), but I plan to add a GUI later. You can exit out of the infinite prompt() loop by clicking cancel.

I'd like feedback on how to improve the code, particularly if there's a better way of doing the getCorrectAns() function, and if I'm using best practices.

var maxNum = 12,
  stillPlaying = true,
  ans, isCorrect = '',
  correctAns = '',
  sign, num1 = 0,
  num2 = 0,
  prob, correctAns;

function randInt(max) {
  return Math.floor(Math.random() * max);
}
function getCorrectAns(n1, sign, n2) {
  switch (sign) {
    case '+':
      return n1 + n2;
    case '\u2212':
      return n1 - n2;
    case '\xD7':
      return n1 * n2;
    case '\xF7':
      return n1 / n2;
    default:
      return false;
  }
}
while (stillPlaying) {
  sign = ['+', '\u2212', '\xD7', '\xF7'][randInt(4)];
  num1 = randInt(maxNum + 1);
  num2 = randInt(maxNum + 1);
  if (sign == '\u2212') num1 += num2;
  if (sign == '\xF7') {
    num2 = randInt(maxNum) + 1;
    num1 *= num2;
  }
  prob = num1 + ' ' + sign + ' ' + num2;
  ans = prompt(isCorrect + prob);
  if (!ans) stillPlaying = false;
  correctAns = getCorrectAns(num1, sign, num2);
  isCorrect = (correctAns == ans) ? 'Correct! ' : 'Incorrect, ' + prob + ' = ' + correctAns + '.\n';
}

\$\endgroup\$
1
  • 3
    \$\begingroup\$ Incorrect, 0 ÷ 0 = NaN. I think division by zero should be excluded! \$\endgroup\$
    – Marc-Andre
    Commented Nov 7, 2014 at 17:38

1 Answer 1

5
\$\begingroup\$

From a once over:

  • You are using lowerCamelCase, that is good
  • Variables like ans and prob deserve to be written out to answer and problem
  • You are using functions, but I think it can be split up a bit more from a best practices. I would go for something like this:

    while( stillPlaying ){
      problem = generateNewProblem();
      answer = getAnswer( problem );
      processAnswer( answer );
    }
    
  • ['+', '\u2212', '\xD7', '\xF7'][randInt(4)]; I wonder why you could not simply use ['+','-','*','/']. Regardless you should have 4 constants for those values since you use those values repeatedly in other places

    var plusSign = '+',
      minusSign = '-',
      timesSign = '*,
      divideSign = '/',
      signs = [plusSign , minusSign, timesSign, divideSign];
    
      if (sign == divideSign) num1 *= num2;
    
  • This seems funny: num1 = randInt(maxNum + 1);, if you were trying to avoid zero, you really need num1 = randInt(maxNum) + 1;
  • Avoid dropping the curly braces and newlines, even when if statements are short like here: if (sign == '\u2212') num1 += num2;

Here's my attempt for fun and giggles.

var maxNum = 12,
    signWidt = 12,
    question = document.getElementById( 'question' ),
    input = document.getElementById( 'input' ),
    errorMessage = document.getElementById( 'errorMessage' ),
    greatMessage = document.getElementById( 'greatMessage' ),
    answer;
//Some of these signs are closer to what children use in school
var plusSign = '+',
    minusSign = '\u2212',
    timesSign = '\u2212',
    divideSign = '\xF7',
    signs = [plusSign , minusSign, timesSign, divideSign];


function randInt(max) {
  return Math.floor(Math.random() * max);
}

function calculateAnswer(n1, sign, n2) {
  switch (sign) {
    case plusSign:
      return n1 + n2;
    case minusSign:
      return n1 - n2;
    case timesSign:
      return n1 * n2;
    case divideSign:
      return n1 / n2;
    default:
      return false;
  }
}

function displayQuestion( s ){
  //We know the question will never surpass 12 chars..
  var extraneousSpace = 12 - s.length, spaceLeft, spaceRight;
  //Compare last bit to 1, if that bit is 1, the number is odd
  //So we subtract 1 before we evenly divide by 2 and 1 space on the right
  if( extraneousSpace & 1 ){
    extraneousSpace--;
    spaceLeft = extraneousSpace / 2;
    spaceRight = spaceLeft + 1;
  } else {
    spaceLeft = spaceRight = extraneousSpace / 2;
  }
  //Mildly evil re-use of variable and magical space repeating routine
  //http://stackoverflow.com/questions/202605/repeat-string-javascript
  spaceLeft = new Array( spaceLeft + 1 ).join( ' ' );
  spaceRight = new Array( spaceRight + 1 ).join( ' ' );
  question.innerText = spaceLeft + s + spaceRight;
}

function generateQuestion(){
  var sign = signs[randInt(4)],
      num1 = randInt(maxNum + 1),
      num2 = randInt(maxNum + 1);  
  
  if (sign == minusSign){
    num1 += num2;
  }
  if (sign == divideSign) {
    num2 = randInt(maxNum) + 1;
    num1 *= num2;
  }  
  answer = calculateAnswer( num1 , sign, num2 );
  displayQuestion( num1 + ' ' + sign + ' ' + num2 )
}

generateQuestion();
input.focus();

input.addEventListener( 'keypress' , function(e){
  console.log( this );
  if( e.which == 13 || e.keyCode == 13 ){
    if( input.value != answer ){
      errorMessage.innerText = 'Wrong, ' + question.innerText.trim() + ' = ' + answer;
      greatMessage.innerText = '';
    }else {
      errorMessage.innerText = '';
      greatMessage.innerText = 'That is right, here\'s another question';    
    }
    input.value = '';
    generateQuestion();
  }
});
#errorMessage { color: red }
#greatMessage { color: green }
<pre>
| ̄ ̄ ̄ ̄ ̄ ̄ |
|   What is  |
|<span id='question'>            </span>| 
|      ?     |
| _______| 
(\__/) || 
(•ㅅ•) || 
/   づ
</pre>
<input type='text' id='input'><br>
<pre>
<span id='errorMessage'></span><span id='greatMessage'></span>
</pre>

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the suggestions! A few clarifications: I used the actual signs because children will probably be unfamiliar with * and / as multiplication and division signs. Also, I am only trying to avoid zero as a denominator of a division problem; I have added a quick fix for that. \$\endgroup\$ Commented Nov 7, 2014 at 19:40

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.