5
\$\begingroup\$

Basically I just wrote this calculator as a sort of JavaScript test for myself. It's a simple calculator with addition, subtraction, and multiplication. I've made some improvements to it before posting this.

At this point I was hoping anyone could just point out some ways I could have cleaned this code up more, basically as an exercise in learning. I've included the JavaScript, followed by the HTML.

    displayNum = "";
    storedNum = "";
    operation = 0;
    queuedOperation = 0;
    calculationFinished = false;

function clearDisplay() {
    // Select the calculator's display
    var display = document.getElementById("display");

    // Clear the global variables and the display
    displayNum = "";
    storedNum = "";
    operation = 0;
    queuedOperation = 0;        
    display.value = displayNum;

}

function numInput(num) {
    // Select the calculator's display
    var display = document.getElementById("display");

    // Check if the display is empty and the number being pressed is 0
    // This is to make sure the first number isn't 0 because then javascript thinks we are using OCTAL (Base eight)
    if ((display.value == "") && num == "0") {
    // If it is, do nothing
      return;
    }
    // Check if a calculation has finished
    // If it has replace the number in the display (the answer to the calculation with the number
    // that was just pressed and change calculation finished back to false 
    else if (calculationFinished == true) {
        display.value = num;
        calculationFinished = false;
    }
    // if neither of these is the case input the numbers as usual
    else {
      display.value += num;
    }
}

function insertDecimal(dec) {
    // Select the calculator's display
    var display = document.getElementById("display");

    // Loop through the current number to make sure there isn't already a decimal
    for (i = 0; i < display.value.length; i++)
        if (display.value.charAt(i) == '.') {
            // If there is, do nothing
            return;
        }
    // If there isn't add a decimal to the end of the displayed number
        display.value += dec;
}

function setOperation(command) {
    // Select the calculator's display
    var display = document.getElementById("display"),
            displayNum = display.value;
    // eval both the numbers to remove quotes
    // otherwise 4 + 5 will be "4" + "5" which in JS will equal 45
            evalDisplay = eval(displayNum),
            evalStored = eval(storedNum);

    // Check if there is a queued operation
    // If there is a queued operation calculate it
    // Then set the stored number to total of the calculation       
    if (queuedOperation == 0) {
        storedNum = display.value;
    }
    else if (queuedOperation == 1) {
        storedNum = evalStored + evalDisplay;
    }
    else if (queuedOperation == 2) {
        storedNum = evalStored - evalDisplay;
    }
    else if (queuedOperation == 3) {
        storedNum = evalStored * evalDisplay;
    }

    // Check what command was put into the calculator
    // Then set the operation to the correct number
    if (command == 'add') {
        operation = 1;
    }
    else if (command == 'subtract') {
        operation = 2;
    }
    if (command == 'multiply') {
        operation = 3;
    }

    // Queue up an operation for enterint multiple  commands without hitting equals
    // i.e. 10x4+8-9+3=
    queuedOperation = operation;
    // Clear the display in order to receive a new number
    display.value = '';
}

function calculate() {
    // Select the calculator's display
    var display = document.getElementById("display");
            displayNum = display.value;
    var evalDisplay = eval(displayNum),
            evalStored = eval(storedNum);

    // Do the math
    if (operation == 1) {
        displayNum = evalStored + evalDisplay;
    }
    else if (operation == 2) {
        displayNum = evalStored - evalDisplay;
    }
    else if (operation == 3) {
        displayNum = evalStored * evalDisplay;
    }
    // Change display to the answer
    display.value = displayNum;
    if (operation != 0)
        calculationFinished = true;
    // Clear all the global variables
    // Necessary in case the user wants to make a calculation using the answer
    operation = 0;
    queuedOperation = 0;
    displayNum = "";
    storedNum = "";
}
<!DOCTYPE html>
    <html>
        <head>
            <script type="text/javascript" src="calculator.js"></script> 
            <link rel="stylesheet" href="style.css" />
        </head>
        <body>
        <form class="calcForm" name="calculator">
            <input type="text" class="calcDisplay" id="display" />
            <div class="calcRow">
                <input type="button" class="calcButton" value="7" onclick="numInput('7')" />
                <input type="button" class="calcButton" value="8" onclick="numInput('8')" />
                <input type="button" class="calcButton" value="9" onclick="numInput('9')" />
                <input type="button" class="calcButton" value="+" onclick="setOperation('add')" />
            </div>
            <div class="calcRow">
                <input type="button" class="calcButton" value="4" onclick="numInput('4')" />
                <input type="button" class="calcButton" value="5" onclick="numInput('5')" />
                <input type="button" class="calcButton" value="6" onclick="numInput('6')" />
                <input type="button" class="calcButton" value="-" onclick="setOperation('subtract')" />
            </div>
            <div class="calcRow">
                <input type="button" class="calcButton" value="1" onclick="numInput('1')" />
                <input type="button" class="calcButton" value="2" onclick="numInput('2')" />
                <input type="button" class="calcButton" value="3" onclick="numInput('3')" />
                <input type="button" class="calcButton" value="x" onclick="setOperation('multiply')" />
            </div>
            <div class="calcRow">
                <input type="button" class="calcButton" value="0" onclick="numInput('0')" />
                <input type="button" class="calcButton" value="." onclick="insertDecimal('.')" />
                <input type="button" class="calcButton" value="C" onclick="clearDisplay()" />
                <input type="button" class="calcButton" value="=" onclick="calculate()" />
            </div>
        </form>
    </body>
</html>

Improve this question
\$\endgroup\$
1
  • \$\begingroup\$ Your four function calculator only implements three functions. \$\endgroup\$
    – Edward
    Sep 22, 2017 at 14:49

3 Answers 3

Reset to default
4
\$\begingroup\$

  • All those globals give me the willies. Same with the evals.

  • You're doing math in two places -- in calculate and in setOperation. Consider that display.value = x; storedNum = display.value; display.value = '' has the same effect as storedNum = x; display.value = ''. Translation: you can call calculate() from setOperation, and avoid repeating yourself. It also removes the need for queuedOperation.

  • insertDecimal loops over the whole string, checking each char for equality with '.'. Better would be if (display.value.indexOf('.') != -1) return;. Or even better than that, if (display.value.indexOf('.') === -1) { /* add the dot */ }.

  • insertDecimal also takes the decimal point as an argument. This doesn't make sense, since you're not using it to add anything but decimal points. Lose the arg.

  • The value of operation isn't obvious. 0, 1, 2, or 3? Those numbers don't mean anything to me. You're not limited to numbers; you could use operator symbols, function names, or even functions themselves. At which point you could get rid of the if/else statements for your math.

  • The global displayNum is never used. In fact, displayNum can be eliminated altogether, as its only purpose is to hold the parsed number. Consider that with the refactoring of operation, you only ever need the value in one place -- and you can convert it right there and avoid having another variable.

  • The display doesn't work quite right when i calculate 125 + 0 or 125 * 0. I get the right result, by happy accident. (An empty string converts to 0.) But the 0 i entered never shows up in the text box.

  • You're repeating yourself by setting the globals at the beginning of the script, and then doing the same thing within clearDisplay. Have an event listener call clearDisplay when the page is ready, and you'll only have one place to change stuff.

  • Stylewise, having the display clear itself the instant you hit an operator button is kinda jarring. Calculators typically don't work that way, and if you're going to make one, you should make it work the way people are used to. Also, the cleared-out box is kinda odd anyway. When you clear, a calculator typically shows '0'.

  • Stylewise, i have a personal hatred of comments that say the obvious. They just add noise.

With that stuff fixed:

function clearDisplay() {
    var display = document.getElementById('display');
    display.value = '0';
    storedNum = '0';
    calculationFinished = true;
    operation = operations.none;
}

function clearPreviousResult() {
    var display = document.getElementById('display');
    if (calculationFinished) {
        display.value = '0';
        calculationFinished = false;
    }
}

function numInput(digit) {
    var display = document.getElementById('display');
    clearPreviousResult();
    // Get rid of a 0 if it's the only thing in there.
    // This particular way of doing it lets you enter a 0 and have it show up,
    // as well as leaving a 0 for the decimal point to snuggle up to.
    if (display.value === '0') display.value = '';
    display.value += digit;
}

function insertDecimal() {
    var display = document.getElementById('display');
    clearPreviousResult();
    if (display.value.indexOf('.') === -1) display.value += '.';
}

operations = {
    // no-op. Takes the right side, and just returns it.  Since the right side is the
    // display value, and calculate() sets display.value, this effectively makes
    // calculate() say "display.value = +display.value".
    none:     function(left, right) { return right; },

    // Math ops.
    add:      function(left, right) { return left + right; },
    subtract: function(left, right) { return left - right; },
    multiply: function(left, right) { return left * right; }
};

function setOperation(command) {
    var display = document.getElementById('display');
    calculate();
    storedNum = display.value;
    if (operations.hasOwnProperty(command))
        operation = operations[command];
}

function calculate() {
    var display = document.getElementById('display');
    display.value = operation(+storedNum, +display.value);
    calculationFinished = true;
    operation = operations.none;
}

if ('addEventListener' in window)
    window.addEventListener('load', clearDisplay);
else
    window.attachEvent('onload', clearDisplay);
Improve this answer
\$\endgroup\$
3
\$\begingroup\$

Replace eval(displayNum) with +displayNum - eval executes the value which not only slower it's vulnerable to XSS.

Other than that you would start to look into making you code Object Oriented (OO) to combine code into logical groups and to take the functions and variables out of the window scope. Good Posts:

https://stackoverflow.com/questions/907225/object-oriented-javascript-best-practices

https://stackoverflow.com/questions/1908443/what-are-good-javascript-oop-resources

Improve this answer
\$\endgroup\$
2
\$\begingroup\$

Some extra items that have not been raised and are so important.

Declare vars

Every variable must have a declaration. If you dont define the variable it is automatically defined and placed in the global scope. This becomes a major source of bugs, headaches, frustration and madness.

var myVar = 0;

Not

myVar = 0;

Event if you want it in global scope.

Use strict mode

All beginners should have as the very first line of any javascript

"use strict";

It will prevent you from doing what what I mentioned above.

Strict equality and inequality

Javascript has two types of equals and not equals. They are == != and === !==. The first two are lazy and will return true if the values are somewhat equal (to hard to explain in a paragraph)

The latter two === and !== are what you should use always, unless you know why you should use the other. Forget you ever saw ==, and != and never use them again.

Blockless blocks.

Blocks after statements can be omitted if you have only one line of code to execute.

 if (something) doFoo();

As opposed to

 if (something) { 
     doFoo();
 }
 // or
 if (something) { doFoo() }

The first without the curlys was introduced some time way back and is the dumbest thing to have ever been allowed in any language. We can not remove it from the languages that allow it, too much source already written.

But you risk your coding sanity if you feel you do not need { } for single lines. Because if you make a change adding another line and forget the curls, you have a bug that is so hard to find, hours and days will be burnt for no good reason but lazy fingers. Just use the rule, all blocks are delimited with {} even if you don't have to.

Improve this answer
\$\endgroup\$