Beginner's JavaScript calculator

Question

As a part of a study-group I did a very simple little JavaScript calculator. It's not made to look pretty.

Any ideas on how I can make the code prettier?

/*
 * This is a simple JS calculator
 * Made by Gemtastic 2016-05-08
 */

// Gather and set up DOM elements
var numberScreen = 'screen';
var numBtns = document.getElementsByClassName('number');
var operators = document.getElementsByClassName('operator');

// Mini statemachine of if we're in the middle of operating on a number.
var operating = false;
var operand = '';

/*
 * Clears the content from the screen.
 */
function clearScreen() {
  document.getElementById(numberScreen).innerHTML = "0";
  resetOperating();
}

/*
 * Evaluates the entered numbers with the given operator.
 */
function evaluate() {
  var result;

  /* If it isn't operating or the last entered is the operator 
   * there's no point in doing anything.
   */
  if(operating && isLastEnteredNumber()){
    var content = document.getElementById(numberScreen).innerHTML;
    var numbers = content.split(operand);

    // Parse strings to be able to operate on them.
    var firstNumber = parseInt(numbers[0], 10);
    var secondNumber = parseInt(numbers[1], 10);

    // Switch the stored operator.
    switch(operand) {
      case '+':
        result = firstNumber + secondNumber;
        break;
      case '÷':
        if(firstNumber !== 0 && secondNumber !== 0) {
          result = firstNumber / secondNumber;
        } else {
          result = 0;
        }
        break;
      case '×':
        result = firstNumber * secondNumber;
        break;
      case '–':
        result = firstNumber - secondNumber;
        break;
      default:
        console.log('Something went terribly wrong! ' + operand + " is not a supported operator!");
    }
  }

  resetOperating();
  document.getElementById(numberScreen).innerHTML = result || 0;
  return result || 0;
}

/*
 * Transforms the current input or the input's value if it's a 
 * full expression but not evaluated into a percentile of itself.
 */
function percent() {
  var content = document.getElementById(numberScreen).innerHTML;
  // Cecks if you've operated two numbers but not yet pressed '=' for evaluation
  if(operating && isLastEnteredNumber()) {
    var result = evaluate();      // Evaluates the screen data
    if(result != '0') {           // if the result isn't 0 make the transformation
      content = content / 100;
    } else {                      // if it is 0 then just append a decimal for user feedback.
      content += '.0';
    }
  } else if (!operating && isLastEnteredNumber && content != '0') { // If you haven't started an operation an a not 0 character do the transformation
      content = content / 100;
  } else {
    content += '.0';              // failsafe append a decimal for user feedback.
    console.log('Bug in percent function! Operating: ' + operating + ', screen content:' + content); // Log, because if this happens we have a bug lulz.
  }
  document.getElementById(numberScreen).innerHTML = content;
  resetOperating();
}

/*
 * Adds a decimal to the screen number.
 */
function decimal() {
  document.getElementById(numberScreen).innerHTML += ".";
}

/*
 * Changes the value to be positive or negative.
 */
function editPositiveNegativeValue() {
    var content = document.getElementById(numberScreen).innerHTML;

    // Simple check if the first character on screen is - or not. If it is it just removes it.
    if(content.charAt(0) == '-') {
      content = content.slice(1);
    } else {
      content = "-" + content;
    }
    document.getElementById(numberScreen).innerHTML = content;
}

/*
 * Resets the state-machine of operation to "off".
 */
function resetOperating() {
  operating = false;
  operand = '';
}

/*
 * Checks if the last character on the screen is a number.
 */
function isLastEnteredNumber() {
  var content = document.getElementById(numberScreen).innerHTML;
  return !isNaN(content.charAt(content.length - 1));
}

/*
 * If the screen isn't showing a simple 0 append
 * to it. Else it will remplace the 0 with the
 * button's number.
 */
function numberClick(event) {
  var content = document.getElementById(numberScreen).innerHTML;
  var btnNum = event.target.innerHTML;
  
  if(content != "0"){
    content += btnNum;
  } else {
    content = btnNum;
  }
  document.getElementById(numberScreen).innerHTML = content;
}

/*
 * Called when you've pressed an operator button.
 */
function operatorClick(event) {
  var operator = event.target.innerHTML;
  var content = document.getElementById(numberScreen).innerHTML;

  switch(operator) {
    case '=':
      evaluate();
      break;
    case 'C':
      clearScreen();
      break;
    case '%':
      percent();
      break;
    case '+/-':
      editPositiveNegativeValue();
      break;
    case '.':
      decimal();
      break;
    default:
      operating = true;
      operand = operator;
      content += operator;
      document.getElementById(numberScreen).innerHTML = content;
      break;
  }
}

// Set up listeners for the operands
for(var o = 0; o < operators.length; o++) {
  operators[o].addEventListener('click', operatorClick, false);
}

// Set up listeners for the buttons
for(var b =  0; b < numBtns.length; b++) {
numBtns[b].addEventListener('click', numberClick, false);
}

clearScreen();

.btn {
  display: inline-block;
  border: 1px solid #ddd;
  padding: 5px;
  margin: 2px;
  border-radius: 5px;
  width: 1em;
  text-align: center;
}

.btn:hover {
  background-color: #eee;
  cursor: pointer;
}

.dummy {
  border: 1px solid #fff;
}
.dummy:hover {
  background-color: transparent;
}

.pad {
  position: absolute;
}

#screen {
  display: inline-block;
  margin-bottom:5px;
  padding: 5px;
  height: 1em;
  width: 90%;
  border-radius: 5px;
  border: 1px solid #ddd;
  text-align: right;
}

<div class="pad">
  <div id="screen"></div>
  <div class="row">
    <div class="btn operator">C</div>
    <div class="btn operator">%</div>
    <div class="btn operator">+/-</div>
    <div class="btn operator">&divide;</div>
  </div>
  <div class="row">
    <div class="btn number">7</div>
    <div class="btn number">8</div>
    <div class="btn number">9</div>
    <div class="btn operator">&#8211;</div>
  </div>
  <div class="row">
    <div class="btn number">4</div>
    <div class="btn number">5</div>
    <div class="btn number">6</div>
    <div class="btn operator">+</div>
  </div>
  <div class="row">
    <div class="btn number">1</div>
    <div class="btn number">2</div>
    <div class="btn number">3</div>
    <div class="btn operator">&times;</div>
  </div>
  <div class="row">
    <div class="btn dummy">&nbsp;</div>
    <div class="btn number">0</div>
    <div class="btn operator">.</div>
    <div class="btn operator">=</div>
  </div>
</div>

Known issues:

• Modulus (%) is not modulus as in 5%2 = 3 it turns the number on the display into a percentage.
• There's no limit to amount of numbers in the display
• It doesn't understand order of operations or many calculations at once. 5 + 56 - 2 = bork. You have to do 5 + 56 = 61 - 2 = 59

Answer 1

Interesting question,

• I noticed that 9-3*3 does not return 0, but 27, you are not following PEMDAS
• Personally, I would make sure that the input only contains numbers, dots, and math operators, and then use eval. eval("9-3*3") does return correctly 0.
• 0.0022 * 2 returns zero.
• I would assign html elements like the one found from document.getElementById(numberScreen) to a higher scope variable for readability and performance
• You do not check for existing periods in the current number, allowing for numbers like 12.24.2016
• Using a pure UI function (innerHTML) to drive the logic is wrong. It's okay for a beginner class, but anything more advanced should drive of (id). You dont want to replace 'x' with '*' and also having to change the JS code
• Your indentation is not perfect, use http://jsbeautifier.org/
• You have zero warnings on jshint.com, which is amazing

Answer 2

To solve "There's no limit to amount of numbers in the display", If you change your javascript to:

if(content != "0"){
    if (content.length < 16)
        content += btnNum;
  } else {
    content = btnNum;
  }

You'll be saying "only add more numbers if there is less than 16 numbers on my screen". (I put 16 because I tested your code and 16 is the max quantity until your div starts to expand)

What do you mean with "Modulus is broken"?

Answer 3

The code looks pretty good. Here are a few suggestions:

You reference document.getElementById(numberScreen) quite a few times. Why not assign it a shorter variable name at the top of your JavaScript file, like you did with numBtns and operators?

You could group your code into sections, to make it easier to find things. For example, have a variable-declaring section, then a functions section, then the code execution section. The sections can be differentiated with comment lines, like:

//--------- functions -------------------

That's just one way to group the code. Another way might work better for you.

You could also group your functions with similar functions, to make them easier to find. As one example, clearScreen() and resetOperating() could be next to each other, and evaluate() could be toward the bottom. While grouping your functions, you can check to make sure no functions are called before they are declared -- it won't make the code run better, but it will increase readability.

Answer 4

code layout

For code cleanliness, I recommend rearranging your code so that the function definitions are all together at the end, after the imperative commands.

// declare variables
// set up event listeners
// clear the screen
// function definitions

avoiding global namespace pollution

Since your logic (wisely) does not depend on global variable declarations, you can avoid polluting the global namespace by wrapping all your code in an immediately executing function expression (IEFE).

(function(){
    // your code here
})();

Since JavaScript variables are function-scoped, this essentially creates a new scope in which your code and functions can refer to variables defined within. Code outside the scope (such as elsewhere on the page) can then use the same variable and function names without running into any collisions.

Answer 5

Fundamentally, your problems lie in the way you are evaluating your input string.

Problem 1:

You only recognize one operator in the string to be evaluated, the last one that was typed. (i.e the contents of operand).

We need something more sophisticated than contents.split(operand) here.

The solution here is to use a lexer. This breaks up the contents string into tokens, much like contents.split except that it will split on more all the different operators, and can also tag each token to say whether it's an operator or a number.

A simple way to do this might be to call contents.split once for each of possible operator, merging the sublists generated each time. In pseudocode:

tokens = contents.split(' ')
for operator in operators {
    tokens = tokens.map(x => x.split(operator))
    tokens = tokens.reduce((a, b) => a + b)
}

The problem with this is that it doesn't give any information on what sort of thing each token is and it isn't very easily extensible.

A better way would be in a state machine:

function lexer(contents) {
    tokens = []
    while contents is not empty {
        char = contents[0]
        contents = contents.slice(1)
        if char is an operator {
            tokens.append({ type: 'operator', value: char })
        }

        number = ""
        while char is a number {
            number += char
            char = contents[0]
            contents = contents.slice(1)
        }

        if number != "" {
            tokens.append({ type: 'number', value: parseInt(number, 10) })
        }
    }
}

This has the advantage of giving information about each token, parsing the numbers at this point rather than later and having far better complexity - \$O(n)\$ instead of \$O(nk)\$ where \$k\$ is the number of operators, and \$n\$ is the length of the string.

Problem 2:

You only use the first two numbers of this in your calculation - firstNumber and secondNumber, and you don't follow the usual orders of operation (PEMDAS - or BIDMAS in the UK)

The solution to this is to parse the tokens we generated in the last step into something which we can evaluate with the right order of operations.

The easiest way to do this is to go through each operator, starting with the highest precedence so that the correct order of operations is maintained:

function parser(tokens) {
    ast = tokens
    for currentOperator in operators, ordered by precedence {
        tokens = ast
        ast = []
        while tokens is not empty {
            token = tokens[0]
            tokens = tokens.slice(1)
            if token is a number {
                ast.append(token)
            } else if token is an operator {
                if token is currentOperator {
                    lhs = ast[ast.length - 1]
                    rhs = tokens[0]
                    ast[ast.length - 1] = 
                        { operator: token, lhs: lhs, rhs: rhs }
                    tokens = tokens.slice(1)
                } else { ast.append(token) }
            }
        }
    }
}

A more sophisticated (and efficient) way to do this would be to use something like Dikjstra's shunting yard algorithm.

This yields what is called an abstract syntax tree (the ast variable from the last example). This is now in a form we can easily evaluate.

There should only be one element in ast if the expression was well formed. This object has three fields: lhs, rhs and operator, which correspond to the left hand side, right hand side and the operator of the operation we should perform. The left and right hand sides are themselves expressions to evaluate, which we should do before performing the operation.

This is expressed in pseudocode like so:

function evaluate(ast) {
    lhs = evaluate(ast.lhs)
    rhs = evaluate(ast.rhs)
    switch ast.operator {
        case '+': return lhs + rhs
        case '-': return lhs - rhs
        case '*': return lhs * rhs
        case '/': return lhs / rhs
    }
}

Putting this all together, our new final solution, that handles multiple operations and evaluates in the correct order, is:

answer = evaluate(parser(lexer(contents))

This pattern of lexing to tokens, parsing tokens to an abstract syntax tree and evaluating or transforming this tree is prevalent in computer science.

Answer 6

Just a few side notes:

To ordinary people, % means percent. Only programmers think of % being the modulus (or remainder, depending on the language) operator. Since your calculator is intended for ordinary people, the % means exactly what it should.

You should add a function to display a result:

function display(result) {
    document.getElementById(numberScreen).innerHTML = result;
}

With this function you don't need to mention numberScreen anywhere else in the code, which is good since the calculator code should talk about numbers and operators, not about manipulating HTML.