Simple Calculator Script

Question

I written my own calculator script in javascript! It is a basic one, but works really well. I am just wondering if there is any way to make this script more efficient & clean. On first look, I feel like I did some redundant stuff in it, however I am a beginner and in no right to judge the code on my own.

window.onload = () => {
  const calculator = document.querySelector('form[name = "calculator"]')
  const btns = document.querySelectorAll(`form[name = "calculator"] table tr input[type = "button"]`)
  btns.forEach((button) => {
    if(button.value != 'c' && button.value != '='){
      button.addEventListener('click', () => {
        calculator.display.value += button.value
      })
    } else {
      if (button.value == 'c') {
        button.addEventListener('click', () => {
          calculator.display.value = '';
        })
      } else if (button.value == '=') {
        button.addEventListener('click', () => {
          calculator.display.value = eval(calculator.display.value);
        })
      }
    }
  })
}

<!DOCTYPE html>
  <html>
    <head></head>
    </body>
      <!-- Page Contents !-->
      <form name = "calculator">
        <table>
          <tr>
            <input type = "text" name = "display" id = "display" disabled>
          </tr>
          <tr>
            <td><input type = "button" name = "one" value = "1"></td>
            <td><input type = "button" name = "two" value = "2"></td>
            <td><input type = "button" name = "three" value = "3"></td>
            <td><input type = "button" name = "plus" value = "+"></td>
          </tr>
          <tr>
            <td><input type = "button" name = "four" value = "4"></td>
            <td><input type = "button" name = "five" value = "5"></td>
            <td><input type = "button" name = "six" value = "6"></td>
            <td><input type = "button" name = "minus" value = "-"></td>
          </tr>
          <tr>
            <td><input type = "button" name = "seven" value = "7"></td>
            <td><input type = "button" name = "eight" value = "8"></td>
            <td><input type = "button" name = "nine" value = "9"></td>
            <td><input type = "button" name = "multiplicatio" value = "*"></td>
          </tr>
          <tr>
            <td><input type = "button" name = "clear" value = "c"></td>
            <td><input type = "button" name = "0" value = "0"></td>
            <td><input type = "button" name = "equal" value = "="></td>
            <td><input type = "button" name = "division" value = "/"></td>
          </tr>
        </table>
      </form>
      <script src = "script.js"></script>
    </body>
  </html>

Are there any ways to make this script "better"? Thanks for the help.

I don't have the time writing a full review, but in general, seeing eval being used should raise a red flag. You don't want to use eval in almost any case. However, changing your code to don't use eval is not trivial though, as you would need to write some sort of simple tokenizer/parser/interpreter to evaluate the expressions entered into the calculator. Your if can also be rewritten using switch ... case. — C5H8NNaO4, Jun 3, 2020 at 8:18
@C5H8NNaO4: eval is not advisable for scripting logic, but for simple mathematical parsing it seems fine. There's no attack vector (it's a local calculator) and it's a clever way to circumvent the complexity of writing your own parser and instead relying one one you have available in JS anyway. — Flater, Jun 3, 2020 at 14:22
It is, however, very easy to crash the program by inputting an improperly formatted equation. Just enter any of the operators not followed by a number and hit =. Some basic error checking should be used to prevent this. Possibly just a matter of adding a try/catch around any use of eval to make sure it parses. But writing a proper parser would be much better and safer in general. — Darrel Hoffman, Jun 3, 2020 at 20:03

RoToRa · Accepted Answer · 2020-06-03 09:00:16Z

You have two errors in the HTML:

A <title> element is required.
There is a closing </body> tag instead of an opening tag <body> tag at the beginning.

Furthermore in the HTML :

The comment  is pointless.
It is convention to write attributes without spaces around the equals sign.
Don't use a (disabled) <input> for output. HTML specifically provides the <output> element for this.
Don't add unnecessary attributes if you don't use them, such as the id on the display or the names on the buttons.

On to the JavaScript:

Don't use the on... event listener properties, but use addEventListener like you did for the button click listener.
It's not wrong, but uncommon to select elements using the name attribute. A class is the usual way.
For the form selector string you correctly use single quotes, but for the button selector string you use a template string with backticks (`) unnecessarily. Use single quotes there too.

Also the button selector is unnecessarily long. Just form[name="calculator"] input[type="button"] is sufficient. Considering you already have a reference to the form you could call querySelectorAll on that:

const calculator = document.querySelector('form[name="calculator"]');
const btns = calculator.querySelectorAll('input[type="button"]');

Looping over all buttons and then assigning the event handlers based on if constraints isn't really practical. It would be better to select the "clear" and "equal" buttons directly and give all numerical and operator buttons a class to select them:

calculator.querySelector('button[name="clear"]').addEventListener("click", ...);

calculator.querySelector('button[name="equal"]').addEventListener("click", ...);

const btns = calculator.querySelectorAll('input.numerical, input.operator').forEach(
    button => button.addEventListener("click", ...);
)

Finally the biggest problem, for two reasons: eval.

Generally using eval is a bad idea. See: Never use eval!
Your program isn't really a calculator. It's an on-screen keyboard that evaluates (potentially invalid) expressions.

At the very least you should replace eval with Function (as the above link demonstrates) and catch any errors it throws. Better would be to check if the entered expression is valid before evaluating it (or even better yet stop the user entering an invalid expression in the first place). Finally try evaluating the expression yourself, or write a application that doesn't build an expression, but calculates as you type just like a real calculator.

Finally, I added the long querySelector because I wanted to be specific and which buttons were the calculator's and which weren't. You are right though, I should have used classes for this — DanielP533, Jun 3, 2020 at 13:28
Also, thanks for the eval understanding. Now I know never to use eval(). I followed a tutorial to get that one. Thanks for the help though. I'm changing the solution to be yours because you helped me with my JS code and didn't just throw some sprinkles at my html. — DanielP533, Jun 3, 2020 at 13:31
@DanielP533 Unfortunately you gain nothing using new Function over eval, it boils down to the very same mechanism. It's not safe(r). — C5H8NNaO4, Jun 3, 2020 at 15:16
@DanielP533 Yes you are right but your code should be readable and maintainable so XYBO's answer also seems correct. — Syed M. Sannan, Jun 3, 2020 at 16:14

Syed M. Sannan · Accepted Answer · 2020-06-03 12:52:54Z

Yes, It can be better in many ways just replace all your code with this:

The first thing I did is made the code formatting just a bit better. then I added a title tag a webpage cannot go on without a title after that I include the meta tags for the UTF-8 encoding and then the viewport meta tag for responsiveness. I also corrected your opening body tag you wrote it as the closing body tag.

Tip: Use vscode as your code editor and add the prettier extension with it to format your code better.

<!DOCTYPE html>
<html>
  <head>
    <!-- These meta tags should be here -->
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <!-- Your page can't go on without a title -->
    <title>Your Page should have a title</title>
  </head>
  <body>
    <!-- Page Contents !-->
    <form name = "calculator">
    <table>
      <tr>
        <input type = "text" name = "display" id = "display" disabled>
      </tr>
      <tr>
        <td><input type = "button" name = "one" value = "1"></td>
        <td><input type = "button" name = "two" value = "2"></td>
        <td><input type = "button" name = "three" value = "3"></td>
        <td><input type = "button" name = "plus" value = "+"></td>
      </tr>
      <tr>
      <td><input type = "button" name = "four" value = "4"></td>
        <td><input type = "button" name = "five" value = "5"></td>
        <td><input type = "button" name = "six" value = "6"></td>
        <td><input type = "button" name = "minus" value = "-"></td>
      </tr>
      <tr>
        <td><input type = "button" name = "seven" value = "7"></td>
        <td><input type = "button" name = "eight" value = "8"></td>
        <td><input type = "button" name = "nine" value = "9"></td>
        <td><input type = "button" name = "multiplicatio" value = "*"></td>
      </tr>
      <tr>
        <td><input type = "button" name = "clear" value = "c"></td>
        <td><input type = "button" name = "0" value = "0"></td>
        <td><input type = "button" name = "equal" value = "="></td>
        <td><input type = "button" name = "division" value = "/"></td>
      </tr>
    </table>
    </form>
    <script src = "script.js"></script>
  </body>
</html>

The Js part

The only thing I did with the Javascript is made the code formatting a lot more readable

window.onload = () => {
  const calculator = document.querySelector('form[name = "calculator"]');
  const btns = document.querySelectorAll(
    `form[name = "calculator"] table tr input[type = "button"]`
  );
  btns.forEach((button) => {
    if (button.value != "c" && button.value != "=") {
      button.addEventListener("click", () => {
        calculator.display.value += button.value;
      });
    } else {
      if (button.value == "c") {
        button.addEventListener("click", () => {
          calculator.display.value = "";
        });
      } else if (button.value == "=") {
        button.addEventListener("click", () => {
          calculator.display.value = eval(calculator.display.value);
        });
      }
    }
  });
};

Oh okay, so all my issues were with the formatting and not the code itself? — DanielP533, Jun 3, 2020 at 0:00
Only formatting the code without at least describing why this is better is off topic for this site. — RoToRa, Jun 3, 2020 at 7:49
@DanielP533 Not just the formatting you did not put the meta tags and did not give it a title and the Js seems fine. — XYBOX, Jun 3, 2020 at 12:12
@RoToRa I have explained my question a bit more now, please check. — XYBOX, Jun 3, 2020 at 12:15
@DanielP533 I have explained my question a bit more now, please check — XYBOX, Jun 3, 2020 at 12:15

Sᴀᴍ Onᴇᴌᴀ · Accepted Answer · 2020-06-03 18:58:49Z

Good Things

For a beginner this looks like a good start. The markup is fairly clean. In the JavaScript code the variables have good scope - i.e. limited to functions, const is used instead of let for variables that don't get re-assigned (which is all variables).

Suggestions

Delegating events

I would recommend using event delegation. Instead of adding click handlers to each button, add a single event handler to the form or another container element and determine the action based on the event.target - e.g. using class names or other attributes. The forEach callback could be altered slightly for this.

Referencing the form in JavaScript

The form can be accessed via a property on Document - i.e. document.forms - so instead of using querySelector()

const calculator = document.querySelector('form[name = "calculator"]')

Access it via the property of document:

const calculator = document.forms.calculator;

This avoids excess DOM queries.

The elements could also be accessed via HTMLFormElement.elements - e.g. document.forms.calculator.elements.clear

Line terminators

Semicolons aren't required for all lines except a handful of statements so as this blog post explains it is best to use them to avoid unintentional behavior in your code.

\$\begingroup\$ Thanks. I'm reading up on Event Delegation right now \$\endgroup\$
– DanielP533
Jun 3, 2020 at 21:40 — DanielP533, Jun 3, 2020 at 21:40
\$\begingroup\$ pastebin.com/NxvVb8MY This is my new script \$\endgroup\$
– DanielP533
Jun 3, 2020 at 22:07 — DanielP533, Jun 3, 2020 at 22:07

Stack Exchange Network

Simple Calculator Script

3 Answers 3

Good Things

Suggestions

Delegating events

Referencing the form in JavaScript

Line terminators

Your Answer

Not the answer you're looking for? Browse other questions tagged
javascript
beginner
html
calculator
dom
or ask your own question.

Hot Network Questions

Simple Calculator Script

3 Answers 3

Good Things

Suggestions

Delegating events

Referencing the form in JavaScript

Line terminators

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
javascript
beginner
html
calculator
dom
or ask your own question.