I've created a Calculator using HTML, CSS and core JS.

I just need some suggestions or improvements on my code.

Here is how my app works:
So, basically, firstly the user enters a number, then clicks on an operator, after then the current number on the screen gets stored in a variable firstOperand and the operator pressed gets stored in a variable currentOperator, then the user enters another number, after then he presses an operator, then the current number on the screen gets stored in a variable secondOperand.

The operator stored in currentOperator now have both the operands, so the answers get shown on the screen, then, the answer gets stored in the firstOperator and the last operator pressed gets stored in the currentOperator and this process continues.

So, if you have any suggestions or improvements for the code, kindly share.

Link to GitHub repo: https://github.com/HarshitSeksaria/Calculator/tree/49ef4b06d2783d96ea8a4c28106a983f665691f6

Code Snippet:

Kindly, switch to full page.

let firstOperatorPressed = false;
let firstOperand = 0;
let secondOperand = 0;
let currentOperator = '';
let operatorPressedRecently = false;

// When user clicks on any numeric button, this function will be executed
function buttonPressed(number) {
    let currentNumber = document.getElementById('screen-text').innerHTML;

    if (currentNumber == '0' || operatorPressedRecently) {
        var changedNumber = number;
    } else {
        var changedNumber = currentNumber + number;
    }

    document.getElementById('screen-text').innerHTML = changedNumber;
    
    operatorPressedRecently = false;
}

// function when any operator is pressed
function buttonOperatorPressed(operator) {
    operatorPressedRecently = true;

    if (firstOperatorPressed) {
        secondOperand = parseInt(document.getElementById('screen-text').innerHTML);

        switch(currentOperator) {
            case '+':
                firstOperand = firstOperand + secondOperand;
                break;
            
            case '-':
                firstOperand = firstOperand - secondOperand;
                break;

            case '*':
                firstOperand = firstOperand * secondOperand;
                break;

            case '/':
                firstOperand = firstOperand / secondOperand;
        }

        document.getElementById('screen-text').innerHTML = firstOperand;
        document.getElementById('history').innerHTML += ' ' + currentOperator + ' ' + secondOperand;
    } else {
        firstOperand = parseInt(document.getElementById('screen-text').innerHTML);
        document.getElementById('screen-text').innerHTML = '0';
        document.getElementById('history').innerHTML = firstOperand;
    }

    firstOperatorPressed = true;
    currentOperator = operator;
}

// function when = is pressed
function buttonEqualPressed() {

    operatorPressedRecently = true;

    if(firstOperatorPressed) {
        secondOperand = parseInt(document.getElementById('screen-text').innerHTML);
        
        switch(currentOperator) {
            case '+':
                firstOperand = firstOperand + secondOperand;
                break;
            
            case '-':
                firstOperand = firstOperand - secondOperand;
                break;

            case '*':
                firstOperand = firstOperand * secondOperand;
                break;

            case '/':
                firstOperand = firstOperand / secondOperand;
        }

        document.getElementById('screen-text').innerHTML = firstOperand;

        firstOperand = parseInt(document.getElementById('screen-text').innerHTML);
        firstOperatorPressed = false;
        document.getElementById('history').innerHTML = document.getElementById('screen-text').innerHTML;
    }
}

function buttonClearPressed() {
    document.getElementById('screen-text').innerHTML = '0';
    firstOperand = 0;
    secondOperand = 0;
    firstOperatorPressed = false;
    currentOperator = '';
    document.getElementById('history').innerHTML = '';
}
/* Styles for whole page */
body {
    font-family: Arial, Helvetica, sans-serif;
    background-image: url(https://i.stack.imgur.com/qg7MO.png);
}

/* Styles for whole calculator */
#wrapper {
    height: 75vh;
    width: 51vh;
    position: absolute;
    top: 0; left: 0; bottom: 0; right: 0;
    margin: auto;
    background-color: #F5F5F5;
    border-radius: 10px;
    animation: fadein 3s;
    background-color: #783393;
}

/* Styles for Heading (Calculator) */
#heading {
    color: #F5F5F5;
    font-size: 2em;
    font-family: 'Segoe UI', Tahoma, Geneva, Verdana, sans-serif;
    text-align: center;
    background-color: #8B78E6;
    padding-block-start: 0.67em;
    padding-block-end: 0.67em;
    margin-block-start: 0em;
    margin-block-end: 0em;
    border-top-left-radius: 10px;
    border-top-right-radius: 10px;
}

/* Styles for history display */
#history {
    min-height: 6vh;
    overflow-x: auto;
    white-space: nowrap;
    text-align: end;
    font-family: consolas;
    font-size: 2em;
    padding: 7px;
    color: #fcfcfc;
    background-color: #FB6F6F;
}

/* Styles for main display */
#screen-text {
    color: #fcfcfc;
    padding: 7px;
    font-family: consolas;
    font-size: 2em;
    text-align: end;
    background-color: #212121;
}

/* Styles for background of History, Main Display and buttons */
#content {
    position: absolute;
    left: 0; right: 0;
    margin: auto;
    width: 51vh;
    background-color: #783393;
    display: inline-block;
    border-radius: 10px;
}

/* Styles for all the buttons */
.button {
    background-color: #783393;
    border: solid 0px #212121;
    color: white;
    width: 12vh;
    height: 12vh;
    cursor: pointer;
    font-family: 'Consolas';
    text-decoration: none;
    font-size: 1.5em;
    transition: 0.5s;
}

/* Style for the button when the mouse is over it */
.button:hover {
    box-shadow: 0px 10px 20px 0px rgba(0,0,0,0.4);
    transform: scale(1.1);
}

/* Animation to fadein the calculator on reload */
@keyframes fadein {
    from { opacity: 0; }
    to   { opacity: 1; }
}
<!DOCTYPE html>
<html>
	<head>
		<meta charset="utf-8" />
		<meta http-equiv="X-UA-Compatible" content="IE=edge">
		<title>Calculator</title>
		<link rel="stylesheet" href="styles.css" />
		<script src="main.js"></script>
	</head>
	<body>
		<div id="wrapper">
			<h1 id="heading">Calculator</h1>
			<div id="content">
				<div id="history"></div>
				<div id="screen-text">0</div>
				<div id="top-section">
						<input type="button" onclick="buttonPressed('7')" class="button" value="7">
						<input type="button" onclick="buttonPressed('8')" class="button" value="8">
						<input type="button" onclick="buttonPressed('9')" class="button" value="9">
						<input type="button" onclick="buttonOperatorPressed('+')" class="button" value="+">
				</div>
				<div id="middle-section">
						<input type="button" onclick="buttonPressed('4')" class="button" value="4">
						<input type="button" onclick="buttonPressed('5')" class="button" value="5">
						<input type="button" onclick="buttonPressed('6')" class="button" value="6">
						<input type="button" onclick="buttonOperatorPressed('-')" class="button" value="-">
				</div>
				<div id="bottom-section">
						<input type="button" onclick="buttonPressed('1')" class="button" value="1">
						<input type="button" onclick="buttonPressed('2')" class="button" value="2">
						<input type="button" onclick="buttonPressed('3')" class="button" value="3">
						<input type="button" onclick="buttonOperatorPressed('*')" class="button" value="*">
				</div>
				<div id="more-section">
						<input style="border-bottom-left-radius: 10px" type="button" onclick="buttonClearPressed()" class="button" value="C">
						<input type="button" onclick="buttonPressed('0')" class="button" value="0">
						<input type="button" onclick="buttonEqualPressed()" class="button" value="=">
						<input style="border-bottom-right-radius: 10px" type="button" onclick="buttonOperatorPressed('/')" class="button" value="/">
				</div>
			</div>
		</div>
	</body>
</html>

up vote 3 down vote accepted

HTML

Current convention is to prefer classes over IDs for CSS.


Try using to structure your HTML more semantically. For one, use semantic elements instead of div everywhere. One example could be output for #screen-text.

Also the calculator buttons are structured in the HTML to represent the physical layout instead of a semantical meaning. Instead consider (simplified):

<section class="calculator__digits">
  <input type="button" value="0">
  <input type="button" value="1">
  <input type="button" value="2">
  <input type="button" value="3">
  <!-- ... --->
  <input type="button" value="7">
  <input type="button" value="8">
  <input type="button" value="9">
</section>

<section class="calculator__operators">
  <input type="button" value="+">
  <input type="button" value="-">
  <input type="button" value="&times;"> <!-- Display proper multiplication/division symbols --> 
  <input type="button" value="&divide;">
</section>

<section class="calculator__equals">
   <input type="button" value="=">
</section

This will require more work in the CSS to have it look like a traditional calculator, but CSS grid and flex box layouts (which modern browsers support) are very powerful and, for example, allow easy reordering and layouting.

At the very least move the in-line border-radius styles into the CSS, e.g.:

#more-section > button:first-child {
   border-bottom-left-radius: 10px
}

#more-section > button:last-child {
   border-bottom-right-radius: 10px
}

Finally event handlers should be be assign in the JavaScript and not via the on... attributes. For example (assuming the digit buttons have the class button--digit):

document.querySelectorAll(".button--digit").forEach(b => {
  b.addEventListener("click", event => {
     buttonPressed(event.target.value);
  });
});

And as a further step you should look into event delegation which allows you to put a single event handler on a parent element instead of separate handlers on each button.

CSS

Not much to say here, except maybe have a look at the different css methodologies that are out there to learn how to structure CSS.

JavaScript

The biggest problem here, is that the code isn't modularized. That means it should be encapsulated in a way that it doesn't potentially interfere with (or is interfered by) other scripts included in the same page. Currently you have multiple global variables and functions. Should another script happen to have one or more global variables or functions with the same name. Have a look at this article (specifically for the start the first section with its four examples) on an introduction to modules.

(Sorry, I have run out of time to write more.)

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.