Calculator with JavaScript

Question

I've just created a fully working calculator for my portfolio, and I would like to get a code review. Does that code looks good, or maybe I should refactor it? Is it good enough to be a part of a portfolio of Junior Front-end Developer?

Here is the working page with it: https://prmk01.github.io/Calculator (There is an option to use the keyboard instead of clicking on a buttons)

The whole project is available on my GitHub. I would appreciate reviewing my whole project. But I'm posting most of the Javascript here, because every post have to contain at least 3 lines of code:

const activatedButtons = document.querySelectorAll('.inactive');
const topRow = document.querySelector('.screen-top-part');
const bottomRow = document.querySelector('.screen-bottom-part');
const basicOperators = /\+|-|÷|×/; 
let expression = '';
let bottomRowActive = true;
let memory = '';

function memoryUpdate (target) {
    bottomRowActive = false;
    for (button of activatedButtons) {
        button.classList.remove('inactive');
    }
    switch (target.innerText) {
        case 'MC': {
            memory = '';
            for (button of activatedButtons) {
                button.classList.add('inactive');
            }
            break
        } case 'MR': {
            clearEntry();
            bottomRow.innerText = memory;
            break
        } case 'M+': {
            memory = eval(+memory + +bottomRow.innerText).toString();
            break
        } case 'M-': {
            memory = eval(+memory - +bottomRow.innerText).toString();
            break
        } case 'MS': {
            memory = bottomRow.innerText;
            break
        }
    }
}

function appendNumber (target) {
    if (topRow.innerText.includes('=')) {
        topRow.innerText = '';
        expression = '';
    }
    if (bottomRowActive) {
        if (bottomRow.innerText.length === 1 && bottomRow.innerText == '0') {
            bottomRow.innerText = target.innerText;
        } else if (bottomRow.innerText.length >= 15) {
        } else {
            bottomRow.innerText = bottomRow.innerText + target.innerText;
        }
        return
    }
    bottomRowActive = true;
    bottomRow.innerText = target.innerText;
}

function clearAll () {
    bottomRowActive = false;
    topRow.innerText = '';
    bottomRow.innerText = '0';
    expression = '';
}

function clearEntry () {
    bottomRowActive = false;
    if (topRow.innerText.includes('=')) {
        clearAll();
    } else {
        bottomRow.innerText = '0';
    }
}

function deleteLast () {
    if (bottomRowActive) {
        if (bottomRow.innerText == '0') {
            return
        } else if ((bottomRow.innerText.length === 3 && bottomRow.innerText.includes('.') && bottomRow.innerText.includes('-')) || (bottomRow.innerText.length === 2 && bottomRow.innerText.includes('-')) || bottomRow.innerText.length === 1) {
            bottomRow.innerText = '0';
        } else {
            bottomRow.innerText = bottomRow.innerText.slice(0, bottomRow.innerText.length - 1)
        }
    }
}

function basicOperation (target) {
    if (bottomRowActive) {
        expression = `(${expression}(${bottomRow.innerText}))`; 
        topRow.innerText = evaluate() + ' ' +  target.innerText;
        expression += target.dataset.value;
        bottomRowActive = false;
    } else {
        if (basicOperators.test(topRow.innerText.slice(-1))) {
            topRow.innerText = topRow.innerText.slice(0, -1) + target.innerText;
            expression = expression.slice(0, -1) + target.dataset.value;
        } else {
            topRow.innerText = evaluate() + ' ' +  target.innerText;
            expression += target.dataset.value;
        }
    }
}

function equals () {
    bottomRowActive = false;
    if (topRow.innerText.includes('=')) {
        return
    }
    if (bottomRow.innerText.includes('-')) {
        topRow.innerText = `${topRow.innerText} (${bottomRow.innerText}) =`;
    } else {
        topRow.innerText = `${topRow.innerText} ${bottomRow.innerText} =`;
    }
    expression = `(${expression}(${bottomRow.innerText}))`;
    bottomRow.innerText = evaluate();
}

function decimalPoint () {
    if (!bottomRowActive) {
        bottomRow.innerText = '0.';
        bottomRowActive = true;
    } else if (bottomRow.innerText.length >= 14) {
    } else if (!bottomRow.innerText.includes('.')) {
        bottomRow.innerText += '.';
    }
}

function minus () {
    if (bottomRow.innerText == '0') {
        return
    } else if (bottomRow.innerText.includes('-')) {
        bottomRow.innerText = bottomRow.innerText.slice(1, bottomRow.innerText.length);
    } else {
        bottomRow.innerText = '-' + bottomRow.innerText;
    }
}

function evaluate () {
    let result = eval(expression);
    let resultRounded = +parseFloat(result).toFixed(8);
    if (resultRounded.toString().length >= 18) {
        resultRounded = resultRounded.toExponential();
    }
    resultRounded = resultRounded.toString();
    if (resultRounded.includes('e')) {
        let number = resultRounded.split('e')[0];
        let numberRounded = (+parseFloat(number).toFixed(8)).toString();
        resultRounded = numberRounded + 'e' + resultRounded.split('e')[1];
    }
    return resultRounded;
}

Answer 1

General points

• DO NOT use let in global scope as it's behavior is non intuitive. If you must use let ensure you create them in a higher scope than global.

• DO NOT add code directly in markup. Eg <button onclick="foo()"></button> use EventTarget.addEventListener to add event listeners and ensure that the page will work even when there is 3rd party code running, Eg adverts, extensions.

• HSchmale's answer tells you

  "...eval() which shouldn't ever be used with user controlled input...".

  In this case there is nothing wrong with using eval as there is no way for it to be used as a vector for attack.

  The user (client) can execute any code they wish, they don't need eval to do so.

  You should NOT use eval when the content being evaluated comes from a 3rd person / party / domain / email / API / etc...

  eval is a useful tool and remains in the ECMAScript standard because it is such a useful tool (A calculator is an example of its usefulness)

• Use textContent rather than innerText

Design

Markup and State

Decouple the Markup from the codes state. You are using the markup to store values. Rather store all the values in code and the markup is just a mirror of the values as stored in code.

The example has the function updateDisplay which is called after each click event. It takes the internal calculator state and displays it.

Use HTMLElement.dataset

You started using the data attributes to define button functions (Great) however you did not complete this.

Generally what is displayed on the page should not be used as defined inputs as there is no guarantee that it will be what you expect.

The HTMLElement.dataset attribute will not be changed by translations, extensions, what not.

Use id to identify elements

Avoid using class names to identify elements.

Class names can change when you make layout and visual changes, they can also often be modified by extensions.

Any changes to the class names means you need to also change the code to accommodate or the app will simply not work.

Rather use id to identify elements

Keep code simple

Use a single listener rather than many independent listeners. This will reduce the code complexity and runtime overheads.

The example uses dataset.value to select a calculator function. The dataset is passed to the function if there is additional data needed to complete the function. For example <button class="button dark" data-func="num" data-num="5">5</button> the function is 'num', and the number is '5'

Example

The code below is not a rewrite as it does not behave as your code does (and likely to have some bugs). Rather it is an example of how you could have organised your code.

Notes

• There is only one event listener buttonClicked.
• Button functions are defined by the object functions where the function to call is in the event targets dataset.func property. Each function is passed the dataset if needed for additional context.
• The display is updated after each click updateDisplay, where the value of result, and current are displayed in the appropriate elements. This is where you would round the displayed numbers etc.
• updateDisplay also checks and updates disabled status of the memory buttons when needed.

;(()=>{  // to keep code out of the global scope
    "use strict";
    var result = "";
    var current = "0";
    var memory;
    var memState = "";
    var error = false; 

    function updateDisplay() {
        function updateMemory() {
            memState = memory;
            const disable = memory === undefined;
            memRecall.disabled = disable;
            memClear.disabled = disable;
        }
        
        calculatorResult.textContent = error ? "Error!" : result;
        error = false;
        calculatorCurrent.textContent = current;
        memory !== memState && updateMemory();          
    }
    const functions = {
        MC() { 
            memory = undefined;
        },
        MR() { 
            current = "" + memory;
        },
        MPlus() { 
            current = "" + ((isNaN(current) ? 0 : Number(current)) + (memory ?? 0)); 
        },
        MSub() { 
            current = "" + ((isNaN(current) ? 0 : Number(current)) - (memory ?? 0)); 
        },
        MS() { 
            memory = isNaN(current) ? 0 : Number(current);
        },
        C() { 
            current = "0";
        },
        CE() {
            current = "0";
            result = "";
        },
        back() { 
            current = current.slice(0,-1);
        },
        equ() {     
            if (!result.includes("=")) {
                try {
                    const val = eval(result + current);
                    result = result + current + "=";
                    current = "" + val;
                } catch(e) {
                    error = true;
                }
            
            }
        },
        op(data) { 
            result = current + data.operator;   
            current = "";
        },
        pluMinus() { 
            current = "" + (current !== "0" ? -Number(current) : current);
        },
        dot() { 
            current += current.includes(".") ? "" : "."; 
        },
        num(data) { 
            current = (current !== "0" ? current : "") + data.num;
        },
    };
    function buttonClicked(e) {
        const target = e.target;
        functions[target.dataset.func]?.(target.dataset);  
        updateDisplay();
    }
    calculatorPanel.addEventListener("click", buttonClicked);
    updateDisplay();
})();

<div class="calculator-body">
  <div class="calculator-screen" >
    <div class="screen-top-part" id="calculatorResult"></div>
    <div class="screen-bottom-part" id="calculatorCurrent">0</div>
  </div>
  <div class="calculator-panel" id="calculatorPanel">
    <div class="panel-top-part" >
      <button class="button top inactive" id="memClear" data-func="MC">MC</button>
      <button class="button top inactive" id="memRecall"  data-func="MR">MR</button>
      <button class="button top" data-func="MPlus">M+</button>
      <button class="button top" data-func="MSub">M-</button>
      <button class="button top" data-func="MS">MS</button>
    </div>
    <div class="panel-bottom-part" id="bottomPanel">
      <button class="button clear" data-func="C">C</button>
      <button class="button" data-func="CE">CE</button>
      <button class="button" data-func="back">←</button>
      <button class="button operator" data-func="op" data-operator="/">÷</button><br>
      <button class="button dark" data-func="num" data-num="7">7</button>
      <button class="button dark" data-func="num" data-num="8">8</button>
      <button class="button dark" data-func="num" data-num="9">9</button>
      <button class="button operator" data-func="op" data-operator="*">×</button><br>
      <button class="button dark" data-func="num" data-num="4">4</button>
      <button class="button dark" data-func="num" data-num="5">5</button>
      <button class="button dark" data-func="num" data-num="6">6</button>
      <button class="button operator" data-func="op" data-operator="-">-</button><br>
      <button class="button dark" data-func="num" data-num="1">1</button>
      <button class="button dark" data-func="num" data-num="2">2</button>
      <button class="button dark" data-func="num" data-num="3">3</button>
      <button class="button operator" data-func="op" data-operator="+">+</button><br>
      <button class="button dark" data-func="pluMinus">±</button>
      <button class="button dark" data-func="num" data-num="0">0</button>
      <button class="button dark" data-func="dot">.</button>
      <button class="button operator equals" data-func="equ">=</button>
    </div>
  </div>
</div>

Answer 2

So I'm looking at it from a structural perspective.

I think you underuse parameters for your functions, and use too much global state. You could benefit from using some Javascript classes to encapsulate it.

You also use eval() which shouldn't ever be used with user controlled input. Don't get in the habit of using it. It's not a big deal on it's own, but it kind of takes away from the point of writing a calculator because you aren't writing any of the parsing yourself.

Your use of JavaScript this could confuse some modern developers, because there's so many ways to end up clobbering it. Most people try to avoid using it as much as possible because of the confusion around it.

Finally you have some empty branches in your code for if conditionals.

Overall, it's a decent first attempt. You may benefit by setting up Github pages to show off your attempt. It's free static hosting and would allow us to view it in action.