12
\$\begingroup\$

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.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Commented Jun 3, 2020 at 8:18
  • \$\begingroup\$ @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. \$\endgroup\$ Commented Jun 3, 2020 at 14:22
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Commented Jun 3, 2020 at 20:03

3 Answers 3

Reset to default
2
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ 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 \$\endgroup\$ Commented Jun 3, 2020 at 13:28
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Commented Jun 3, 2020 at 13:31
  • \$\begingroup\$ is window.Function() safe? \$\endgroup\$ Commented Jun 3, 2020 at 13:43
  • 1
    \$\begingroup\$ @DanielP533 Unfortunately you gain nothing using new Function over eval, it boils down to the very same mechanism. It's not safe(r). \$\endgroup\$ Commented Jun 3, 2020 at 15:16
  • \$\begingroup\$ @DanielP533 Yes you are right but your code should be readable and maintainable so XYBO's answer also seems correct. \$\endgroup\$ Commented Jun 3, 2020 at 16:14
5
\$\begingroup\$

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);
        });
      }
    }
  });
};
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Oh okay, so all my issues were with the formatting and not the code itself? \$\endgroup\$ Commented Jun 3, 2020 at 0:00
  • 3
    \$\begingroup\$ Only formatting the code without at least describing why this is better is off topic for this site. \$\endgroup\$ Commented Jun 3, 2020 at 7:49
  • \$\begingroup\$ @DanielP533 Not just the formatting you did not put the meta tags and did not give it a title and the Js seems fine. \$\endgroup\$ Commented Jun 3, 2020 at 12:12
  • \$\begingroup\$ @RoToRa I have explained my question a bit more now, please check. \$\endgroup\$ Commented Jun 3, 2020 at 12:15
  • \$\begingroup\$ @DanielP533 I have explained my question a bit more now, please check \$\endgroup\$ Commented Jun 3, 2020 at 12:15
1
\$\begingroup\$

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.

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

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.