9
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ Possible bug :- 1 / 3 * 3 gives 3. \$\endgroup\$ – A---B Dec 21 '16 at 18:26
  • 2
    \$\begingroup\$ Definite bug: 0.3 x 3 = 0 \$\endgroup\$ – Kyeotic Dec 21 '16 at 18:31
  • \$\begingroup\$ It's not made to support automatic correct order of calculations so it's not a bug, it's also probably because of your browser's JS interpreter, but the 0.3 x 3 is indeed a bug as I never implemented decimals :S \$\endgroup\$ – Gemtastic Dec 21 '16 at 18:33
  • 1
    \$\begingroup\$ 5+56-2 == 3 what now? \$\endgroup\$ – njzk2 Dec 21 '16 at 20:15
  • \$\begingroup\$ @njzk2 It doesn't handle many operations in one go either. Simple claculator, but I should probably fix that :) Updated question to clarify this. \$\endgroup\$ – Gemtastic Dec 21 '16 at 21:07
6
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ "You have zero warnings on jshint.com, which is amazing". Indeed \$\endgroup\$ – Kyeotic Dec 21 '16 at 18:32
  • \$\begingroup\$ "anything more advanced should drive of (id)." -- Was this sentence supposed to be refactored or something, because i don't get it. \$\endgroup\$ – Scimonster Dec 21 '16 at 19:35
  • 3
    \$\begingroup\$ 9-3*3 giving 27 isn't even a PEMDAS issue as far as I can tell. (9-3)*3 would give 18. I'm not exactly sure how it's parsing 9-3*3. \$\endgroup\$ – Dason Dec 21 '16 at 19:57
  • 1
    \$\begingroup\$ I'm not sure how it parses 9-3*3 either because I obviously haven't implemented multiple calculations :P I do get the ID part. \$\endgroup\$ – Gemtastic Dec 21 '16 at 21:21
4
\$\begingroup\$

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"?

\$\endgroup\$
  • \$\begingroup\$ Modulus doesn't do modulus, it converts the number to percent. \$\endgroup\$ – Gemtastic Dec 21 '16 at 16:58
  • \$\begingroup\$ @Gemtastic Sorry, I still don't get it. In your code, it seems like you want to % convert do percent, but you actually want to it calculate modulus? Or you were only warning us that you "changed" the function of the symbol "%"? \$\endgroup\$ – Cris Motinha Dec 21 '16 at 17:05
  • \$\begingroup\$ It is an intentional choice, to make the button do SOMETHING but modulous is a different operator that does different calculations :) \$\endgroup\$ – Gemtastic Dec 21 '16 at 17:19
  • \$\begingroup\$ Oh, that's fine. In all calculators "%" mean percentage, so that's okay :) I just don't get why you said it was broken... Your percentage function is broken, is that it? \$\endgroup\$ – Cris Motinha Dec 21 '16 at 17:21
  • 2
    \$\begingroup\$ Is there anyway to try 5%2? Because at the exactly time you click % you get the percentage result. I don't think you should call it a broken thing, although you could create an "help sheet" (where you could say that % is percentage, not modulus) :) \$\endgroup\$ – Cris Motinha Dec 21 '16 at 17:36
3
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\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.