Simple Calculator Script

By Posted on

Problem

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>

Expand snippet

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

Solution

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 <!-- Page Contents !--> 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.

  1. Generally using eval is a bad idea. See: Never use eval!

  2. 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.

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);
        });
      }
    }
  });
};

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.

Leave a Reply

Your email address will not be published. Required fields are marked *