8
\$\begingroup\$

I decided to write a very simple web-game:

function write_at(x, y, text, color, font) {
  ctx.fillStyle = color || "black";
  ctx.font = font || "50px Arial";
  ctx.fillText(text, x + 50, y + 50);
}

function write_at_random(text, color, font) {
  write_at(random_choice(range(0, canvas.width - 250)),
    random_choice(range(200, canvas.height - 250)),
    text, color, font);
}

function random_choice(array) {
  index = Math.random() * array.length
  return array[Math.floor(index)]
}

function range(start, end) {
  return Array.apply(0, Array(end))
    .map(function(element, index) {
      return index + start;
    });
}

function gen_math_expression() {
  start = random_choice(range(0, 20));
  operator = random_choice(['+', '-', '*']);
  end = random_choice(range(0, 20));
  return start + operator + end;
}

function single_digit(expr) {
  return eval(expr) <= 9 && eval(expr) >= 1
}

function gen_single_digit_expression() {
  expr = gen_math_expression();
  while (!single_digit(expr)) {
    expr = gen_math_expression();
  }
  return expr;
}

function number_from_keycode(keycode) {
  return keycode.which - 48;
}

function draw_welcome_and_score() {
  write_at(0, 0, "Single Digit Math Quiz");
  write_at(600, 0, points);
}

function lose_sound() {
  var snd = new Audio("http://www.soundjay.com/misc/sounds/fail-buzzer-02.mp3");
  snd.play();
}

function win_sound() {
  var snd = new Audio("http://www.soundjay.com/misc/bell-ringing-05.mp3");
  snd.play();
}



function main() {
  canvas = document.getElementById("canvas")
  ctx = canvas.getContext('2d');
  points = 0
  draw_welcome_and_score()
  write_at(0, 100, "Press the result of the operation on your keyboard.", 0, "20px Arial");

  expr = gen_single_digit_expression()
  write_at_random(expr);

  function check_expr_and_go_next(e) {
    if (number_from_keycode(e) == eval(expr)) {
      points++;
      color = '#7CFC00'; // light green
      win_sound()
    } else {
      points--;
      color = 'red';
      lose_sound()
    }
    ctx.clearRect(0, 0, canvas.width, canvas.height)
    canvas.style.background = color;
    draw_welcome_and_score()
    expr = gen_single_digit_expression()
    write_at_random(expr);
  }

  window.addEventListener("keydown", check_expr_and_go_next, true);


}

main()
<html>
<canvas id="canvas" width="800" height="600" style="border:1px solid #000000;">
</canvas>
</html>

\$\endgroup\$
  • 2
    \$\begingroup\$ That's pretty neat! \$\endgroup\$ – Phrancis May 25 '15 at 21:32
  • \$\begingroup\$ Why do the questions appear in random places on the screen? \$\endgroup\$ – 200_success May 25 '15 at 21:36
  • \$\begingroup\$ @200_success to engage the younger ones, that will be happy seeing numbers all over the place instead of only one place. \$\endgroup\$ – Caridorc May 25 '15 at 21:49
  • \$\begingroup\$ The method you use to pick the expressions results in most being subtraction. You might want to consider picking the first number and the operator at random then selecting the second number from the range that produces results under 10. This would give you more control over the proportion of each type of expression being generated. \$\endgroup\$ – Bob May 27 '15 at 6:42
2
\$\begingroup\$

I don't see a var anywhere in your code. That means that all of your variables are global variables.

Constructing an array of integers just to pick a random element seems wasteful. The following code scales better, and in my opinion, is easier to follow because it avoids the unnecessary intermediate array.

Asking whether an expression is single digit is a bit odd. I would have the caller evaluate the expression first — which also has the advantage of evaluating it just once.

I'm not convinced that a canvas is called for. Since it's all just text, the whole game can be implemented using the usual DOM elements. The styling should then be done using CSS.

The code supports the number keys along the top row of the keyboard, but it should support the numeric keypad as well.

/* Picks a random integer n such that min ≤ n < max. */
function randomInt(min, max) {
    return min + Math.floor((max - min) * Math.random());
}

function sample(candidates) {
    return candidates[randomInt(0, candidates.length)];
}

function genMathExpression() {
    return randomInt(0, 20) + sample('+-*') + randomInt(0, 20);
}

function isSingleDigit(number) {
    return 1 <= number && number <= 9;
}

function genSingleDigitExpression() {
    do {
        var expr = genMathExpression();
    } while (!isSingleDigit(eval(expr)));
    return expr;
}

function ask(text) {
    var q = document.getElementById('question');
    var top = q.previousSibling.offsetTop;
    var bottom = q.parentNode.offsetTop + q.parentNode.offsetHeight;
    var left = q.parentNode.offsetLeft;
    var right = q.parentNode.offsetWidth;
    q.style.top = randomInt(top, bottom - 60) + 'px';
    q.style.left = randomInt(left, right - 120) + 'px';
    q.textContent = text;
}

function numberFromKeycode(keycode) {
    var code = keycode.which;
    return (48 <= code && code < 58)  ? code - 48 :  // top row of keys
           (96 <= code && code < 106) ? code - 96 :  // numeric keypad
           null;
}

function main() {
    var points = 0;
    var expr;
    ask(expr = genSingleDigitExpression());

    function handleKeypress(event) {
        if (numberFromKeycode(event) == eval(expr)) {
          document.getElementById('score').textContent = ++points;
          document.getElementById('board').className = 'correct';
          new Audio("http://www.soundjay.com/misc/bell-ringing-05.mp3").play();
        } else {
          document.getElementById('score').textContent = --points;
          document.getElementById('board').className = 'incorrect';
          new Audio("http://www.soundjay.com/misc/sounds/fail-buzzer-02.mp3").play();
        }
        document.getElementById('instructions').style.display = 'none';
        ask(expr = genSingleDigitExpression());
    }

    window.addEventListener("keydown", handleKeypress, true);
}

main();
#board {
    border: 1px solid black;
    width: 800px;
    height: 600px;
    padding: 10px 50px;
    color: black;
    font: 50px Arial, sans-serif;
    position: relative;
}

#board.correct {
    background-color: #7cfc00; /* light green */
}

#board.incorrect {
    background-color: red;
}

h1, #score {
    display: inline;
    font-size: 50px;
    font-weight: normal;
}

#score {
    display: inline;
    float: right;
}

#instructions {
    margin-top: 3em;
    font-size: 20px;
}

hr {
    visibility: hidden;
}

#question {
    position: absolute;
}
<div id="board">
  <h1>Single Digit Math Quiz</h1>
  <div id="score">0</div>
  <div id="instructions">
    Press the result of the operation on your keyboard.
  </div>
  <hr id="hr"><div id="question"></div>
</div>

\$\endgroup\$
  • \$\begingroup\$ Nice point about the globals, but when you say wasteful, I don't understand. The code should run only very few times, efficiency does not matter. \$\endgroup\$ – Caridorc May 26 '15 at 19:02
  • \$\begingroup\$ Anyway It really is easier to follow that we first define a random_range function and then a random_choice building from that :) \$\endgroup\$ – Caridorc May 26 '15 at 20:33
5
\$\begingroup\$

You can't currently play with the numbers on the numpad of a keyboard - they have different keycodes to the numbers along the top.

It's normal to camelCase function names rather than snake_case.

Avoid abbreviations e.g. the gen in gen_single_digit_expression.

function number_from_keycode(keycode) { - keycode isn't a keycode at all, it's an event.

Try to remember your semicolons - it's not required but many people (myself included) prefer to see them. There are a couple of very subtle edge cases that can cause problems if you omit them.

eval is evil. You really don't need to invoke the interpreter to do a simple sum!

Here's an example of a simple constructor function (and usage) to show you a way of doing it without eval.

var MathExpression = function(left, operator, right) {
    this.evaluate = function () {
        switch (operator) {
            case '+':
                return left + right;
            case '-':
                return left - right;
            case '*':
                return left * right;
        }
    };
};

var e1 = new MathExpression(1, '+', 8);
console.log(e1.evaluate()); // 9

You don't need to brute force expression generation either. Choose the operator first, then choose the LHS and finally choose a number for the RHS that is within the allowed range.

'+' (addition)
Choose LHS: 1 <= LHS <= 8
Choose RHS: 1 <= RHS <= 9 - LHS

LHS choose 5.
RHS can be 1, 2, 3 or 4 to yield a single digit answer.

You can do the same for the other operators:

'*' (multiplication)
Choose LHS: 1 <= LHS <= 4
Choose RHs: 1 <= RHS <= 9/LHS (rounded down).

LHS choose 4.
RHS can be 1 or 2 to yield a single digit number. (9/4 = 2.25 -> 2)

I'll leave '-' for you to do.

\$\endgroup\$
  • 2
    \$\begingroup\$ You say eval is evil but is this the universal truth? Eval is dangerous, but it is no more evil than the gunpowder is. In this case it saves me tons of code and is never run on any input, only strings generated by me. Also do you think I should ditch bruteforce?, performance is not a concern whatsover. \$\endgroup\$ – Caridorc May 26 '15 at 19:05
  • \$\begingroup\$ @Caridorc - not a universal truth but a notable quote from Crockford \$\endgroup\$ – RobH May 26 '15 at 20:09
  • 1
    \$\begingroup\$ in fact you are mostly right (+1) but evaluating a single mathematical expression is in my opinion one of the very few legit uses of eval. \$\endgroup\$ – Caridorc May 26 '15 at 20:26
1
\$\begingroup\$

The range function is misleading. From its parameter names I would guess that range(3, 5) returns [3, 4] (or possibly [3, 4, 5]), but actually it gives [3, 4, 5, 6, 7]. Since most of times you use it with 0 as the first parameter, your intention is not very clear, and this may be a bug.

This implementation would behave more like what I'd expect (exclusive end):

function range(start, end) {
  return Array.apply(0, Array(end - start))
    .map(function(element, index) {
      return index + start;
    });
}

Or if you prefer the range to be inclusive, then change to end - start + 1.

\$\endgroup\$

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.