1
\$\begingroup\$

I built a simple calculator that supports addition, subtraction, multiplication, and division. It can also chain operations. I'm looking for feedback on my code for places that could be done better in terms of either efficiency or best code practices.

$(function() {
  'use strict';

  var calculator = (function() {
    var operators = [],
        numbers = [],
        numberString = '';

    // cache the dom
    var app = document.querySelector('.calcapp'),
        output = app.querySelector('.entry');

    // bind events
    app.addEventListener('mouseup', function(event) {
      var id = event.target.id;

      if (!isNaN(parseInt(id, 10)) || id === '.') {
        numberString += id;
        render(numberString);
      } else if (id === '+' || id === '-' || id === 'x' || id === '/' || id === '=') {
        if (numberString !== '' && id !== '=') {
          numbers.push(parseFloat(numberString, 10));
          numberString = '';
          operators.push(id);
          render(numbers[numbers.length - 1]);
        } else if (id !== '=') {
          operators.pop();
          operators.push(id);
          render(numbers[numbers.length -1]);
        } else if (id === '=') {
          numbers.push(parseFloat(numberString, 10));
          var total = loopOverOperators();
          numberString = total.toString();
          numbers = [];
          operators = []
          render(total);
        }

      } else if (id === 'clear') {
        clear();
        render();
      }
    });

    render();

    function clear() {
      operators = [];
      numbers = [];
      numberString = '';
    }

    function calculateTotal(current, number, operator) {
      var newTotal= current;

      switch (operator) {
        case '+':
          newTotal += number;
          break;
        case '-':
          newTotal -= number;
          break;
        case 'x':
          newTotal *= number;
          break;
        case '/':
          newTotal /= number;
          break;
        default:
          break;
      }

      return newTotal;
    }

    function loopOverOperators() {
      var newTotal = numbers[0];

      for (var i = 0, length = operators.length; i < length; i++) {

        newTotal = calculateTotal(newTotal, numbers[i + 1], operators[i]); 
      }

      return newTotal;
    }

    function formatOutput() {
      var newInput = [];

      for (var i = 0, length = operators.length; i < length; i++) {
        newInput.push(numbers[i]);
        newInput.push(' ');
        newInput.push(operators[i]);
        newInput.push(' ');
      }

      return newInput;
    }

    function render(total) {
      var htmlString = '',
          total = total !== undefined ? total : 0;

      htmlString += formatOutput().join('');
      htmlString += '<br/>';
      htmlString += '<span class="total">' + total.toString() + '</span>';

      output.innerHTML = htmlString;
    }

  })();
});

HTML:

<header class='header  center'>Calculator</header>
<section class='calcapp'>
  <div class='entry'></div>
  <div>
    <button class='clear' id='clear'>Clear</button>
    <button class='btn' id='/'>/</button>
  </div>

  <div class='topRow'>
    <button class='btn' id='7'>7</button>
    <button class='btn' id='8'>8</button>
    <button class='btn' id='9'>9</button>
    <button class='btn' id='x'>x</button>
  </div>

  <div class='middleRow'>
    <button class='btn' id='4'>4</button>
    <button class='btn' id='5'>5</button>
    <button class='btn' id='6'>6</button>
    <button class='btn' id='-'>-</button>
  </div>

  <div class='bottomRow'>
    <button class='btn' id='1'>1</button>
    <button class='btn' id='2'>2</button>
    <button class='btn' id='3'>3</button>
    <button class='btn' id='+'>+</button>
  </div>

  <div>
    <button class='zero' id='0'>0</button>
    <button class='btn' id='.'>.</button>
    <button class='btn' id='='>=</button>
  </div>

</section>

CSS:

html,
body {
  margin: 0;
  padding: 0;
}

body {
  background-color: #fafafa;
  color: #333;
  font-family: 'Open Sans', sans-serif;
  font-size: 18px;
  line-height: 125%;
  max-width: 300px;
}

button {
  background-color: #95a5a6;
  border: none;
  font-size: 20px;
  margin: 5px 0px;
  outline: none;
  padding: 15px 0;
  cursor: pointer;
}

div {
  width: 100%;
}

.calcapp {
  background-color: #34495e;
  padding: 10px 0 5px 10px;
  min-width: 308px;
}

.center {
  text-align: center;
}

.header {
  font-size: 24px;
  font-weight: bold;
  margin: 25px 0 15px 0;
}

.entry {
  background-color: #fafafa;
  padding: 15px 0;
  margin-bottom: 5px;
  text-align: right;
  width: 96.5%;
}

.total {
  font-size: 1.3em;
  font-weight: bold;
}

.btn {
  box-sizing: border-box;
  min-width: 48px;
  width: 23%;
}

.clear {
  width: 72%;
}

.zero {
  width: 47.5%;
}

@media screen and (min-width: 360px) {
  body {
    margin: 0 auto;
  }
}
\$\endgroup\$
2
\$\begingroup\$

Very neat.

You could lose jQuery by writing your own ready() function which would save about 95KB (if you used jQuery 1.12).

On a sidenote

I would make two functional changes:

  • JavaScript returns the property Infinity when you divide by 0 which is mathematically incorrect. I would replace that by giving out something like "Math ERROR".
  • I would also reset the calculator
    • IF = is followed by a number input
    • BUT NOT IF = is followed by another operator

Here is a fiddle with said changes.


EDIT

There are some other mathematical flaws:

  • When chaining operations without using = between the steps you just iterate through the operators and numbers which doesn't account for the order of operations. You can check what I mean with the following order of inputs:

    2 + 2 * 3 = which will return 12 when it should return 8.

    This could be fixed by either following the order of operations (first calculating the multiplication and division operations from left to right then the addition and subtraction operations) or by calculating and displaying a currentTotal after each operator.

  • You should also account for leading operators (most often -).

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