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;
}
}
1 Answer 1
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 by0
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 anumber
input - BUT NOT IF
=
is followed by anotheroperator
- IF
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 theoperators
andnumbers
which doesn't account for the order of operations. You can check what I mean with the following order ofinputs
:2 + 2 * 3 =
which will return12
when it should return8
.This could be fixed by either following the order of operations (first calculating the
multiplication
anddivision
operations from left to right then theaddition
andsubtraction
operations) or by calculating and displaying acurrentTotal
after eachoperator
.You should also account for leading
operators
(most often-
).
Explore related questions
See similar questions with these tags.