7
\$\begingroup\$

This is a simple calculator made in 2/3 hours. I am still new and I was trying out some tags. If you can add some comment and say what to do and what to improve I will be really grateful.

var buttonOne = document.forms.formularzCalc.keyOne;
var buttonTwo = document.forms.formularzCalc.keyTwo;
var buttonThree = document.forms.formularzCalc.keyThree;
var buttonFour = document.forms.formularzCalc.keyFour;
var buttonFive = document.forms.formularzCalc.keyFive;
var buttonSix = document.forms.formularzCalc.keySix;
var buttonSeven = document.forms.formularzCalc.keySeven;
var buttonEight = document.forms.formularzCalc.keyEight;
var buttonNine = document.forms.formularzCalc.keyNine;
var buttonZero = document.forms.formularzCalc.keyZero;
var buttonPlus = document.forms.formularzCalc.keyPlus;
var buttonMinus = document.forms.formularzCalc.keyMinus;
var buttonMulti = document.forms.formularzCalc.keyMulti;
var buttonDivide = document.forms.formularzCalc.keyDivide;
var buttonCancel = document.forms.formularzCalc.keyCancel;
var buttonResult = document.forms.formularzCalc.keyResult;
var resultArea = document.forms.formularzCalc.Wynik;
var znacznik = 0;
var x;
var y = "";



// klawisze
function LiczbaX() {
  resultArea.value += this.value
}

function LiczbaY() {
  if (resultArea.value != "")
    resultArea.value += this.value
}
// operatory arytmetyczne i przypisywanie wartości resultArea do x
function znak() {
  x = resultArea.value;
  resultArea.value = "";
  znacznik = this.value;
}

// przypisywanie wartości resultArea do y i obliczanie
function wyniczek() {
  y = resultArea.value;
  if (y == "")
    alert("Co do **** karmazyna ?!?");
  else {
    switch (znacznik) {
      case "+":
        if (isNaN(x) == false && isNaN(y) == false)
          resultArea.value = parseInt(x) + parseInt(y);
        else
          alert("Thats wrong! use C and try again");
        break;

      case "-":
        if (isNaN(x) == false && isNaN(y) == false)
          resultArea.value = parseInt(x) - parseInt(y);
        else
          alert("Thats wrong! use C and try again");
        break;

      case "*":
        if (isNaN(x) == false && isNaN(y) == false)
          resultArea.value = parseInt(x) * parseInt(y);
        else
          alert("Thats wrong! use C and try again");
        break;

      case "/":
        if (y == 0 || isNaN(x) == true || isNaN(y) == true)
          alert("Thats wrong! use C and try again");
        else
          resultArea.value = parseInt(x) / parseInt(y);
        break;
    }
  }
}
// zdarzenia kliknięć cyfr
buttonOne.addEventListener("click", LiczbaX)
buttonTwo.addEventListener("click", LiczbaX)
buttonThree.addEventListener("click", LiczbaX)
buttonFour.addEventListener("click", LiczbaX)
buttonFive.addEventListener("click", LiczbaX)
buttonSix.addEventListener("click", LiczbaX)
buttonSeven.addEventListener("click", LiczbaX)
buttonEight.addEventListener("click", LiczbaX)
buttonNine.addEventListener("click", LiczbaX)
buttonZero.addEventListener("click", LiczbaY)

// zdarzenia kliknięć cyfr
buttonPlus.addEventListener("click", znak)
buttonMinus.addEventListener("click", znak)
buttonMulti.addEventListener("click", znak)
buttonDivide.addEventListener("click", znak)

// zdarzenie podawania wyniku
buttonResult.addEventListener("click", wyniczek);
body {
  background-color: #fff0b7;
}


/* Edytowanie formularza */

form {
  width: 40%;
  margin: auto;
  text-align: center;
}

td input {
  width: 40px;
  height: 30px;
}

fieldset {
  border-color: #00998C;
  background-color: rgba(0, 0, 0, 0.05);
}

legend:first-letter {
  color: #DDcc67;
  font-size: 120%;
}

legend {
  background-color: #00998C;
  padding: 5px 15px;
  border-radius: 15px;
  font-size: 23px;
  font-weight: bolder;
  color: #eeeeee;
  font-family: "Arial Black";
  margin: 0 auto;
  text-shadow: 3px 3px 1px #333333;
}


/*  Edytowanie tabeli */

table {
  margin: auto;
}

th input {
  text-align: right;
}


/* reszta */

footer {
  width: 40%;
  background-color: #00998C;
  margin: 5px auto;
  text-align: center;
  border: 1px rgba(0, 0, 0, 0.30) solid;
  color: #EEEEEE;
  font-family: verdana;
  font-weight: bold;
  font-size: 80%;
  min-width: 250px;
  border-radius: 10px;
  padding: 3px 0px;
}

input[disabled] {
  box-shadow: 1px 1px 1px black;
}

img {
  position: absolute;
  left: 20px;
  top: 20px;
}
<!DOCTYPE html>
<html>

<head>
  <meta charset="utf-8">
  <title> Kalkulator </title>
  <link rel="stylesheet" href="kalkulatorCss.css" type="text/css">
</head>

<body>
  <form name="formularzCalc">
    <fieldset>
      <legend> Calc Upgraded version </legend>
      <table>
        <!-- Tabela do wyrównania zawartości kalkulatora -->
        <tr>
          <th colspan="4"><input type="text" name="Wynik" disabled></th>
          <!-- Użycie th w celu łatwiejszej edycji -->
        </tr>
        <tr>
          <td><input type="button" name="keySeven" value="7"></td>
          <td><input type="button" name="keyEight" value="8"></td>
          <td><input type="button" name="keyNine" value="9"></td>
          <td><input type="button" name="keyPlus" value="+"></td>
        </tr>
        <tr>
          <td><input type="button" name="keyFour" value="4"></td>
          <td><input type="button" name="keyFive" value="5"></td>
          <td><input type="button" name="keySix" value="6"></td>
          <td><input type="button" name="keyMinus" value="-"></td>
        </tr>
        <tr>
          <td><input type="button" name="keyOne" value="1"></td>
          <td><input type="button" name="keyTwo" value="2"></td>
          <td><input type="button" name="keyThree" value="3"></td>
          <td><input type="button" name="keyMulti" value="*"></td>
        </tr>
        <tr>
          <td><input type="reset" name="keyCancel" value="C"></td>
          <td><input type="button" name="keyZero" value="0"></td>
          <td><input type="button" name="keyResult" value="="></td>
          <td><input type="button" name="keyDivide" value="/"></td>
        </tr>
      </table>
    </fieldset>
  </form>
  <footer>
    The new Calc made by Hyakkimaru <sup>&copy;</sup> Wszelkie prawa zastrzeżone
  </footer>
  
  <script src="kalkulatorJavaScript.js"></script>
</body>

</html>

\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to Code Review! I hope you get some good answers. \$\endgroup\$ – Phrancis Mar 13 at 19:26
  • \$\begingroup\$ You could add another feature. For example, if I press 2 + 3 + 1 and then equals, I get 4. It only adds the last two numbers you entered. Apart from this it seems to work pretty good. \$\endgroup\$ – Dennis Fink Mar 20 at 18:27
  • \$\begingroup\$ @hyakkimaru Still wanting a review? I see this is pretty old \$\endgroup\$ – dustytrash Apr 1 at 2:04
2
\$\begingroup\$

General feedback

For a beginner the code is okay - for the most part it appears to function like a calculator, except for the fact that if I click the sequence4, +, C, 5, = then the result shows 5, whereas with your calculator I see 9 because clearing the form via the reset/cancel button (i.e. C) doesn't clear the value in the variable x.

This code is quite redundant - which is covered in more detail below. Also, some of the prompts, variables and method names appear to be in Polish while others are in English. It is best to choose one and be consistent.

Targeted feedback

Repeated code

This code does not adhere to the Don't Repeat Yourself principle - thus some would say the code is wet. When creating larger applications, there may be many more elements on a page that need to have interactions handled and it would take a long time to copy/paste references and event handlers.

There are various ways to avoid such redundancy. One technique would be to name the integer inputs with the integer number instead of the cardinal number (e.g. key1, key2, etc.) and then loop through the elements and add event handlers - something like:

for (var i = 1; i < 10; i++ ) {
  document.forms.formularzCalc["key" + i].addEventListener("click", LiczbaX)
}

Another approach would be to use Event delegation to handle clicks on a container element and then delegate actions based on the target element that was clicked. With that approach there is no need to add the event listener to all elements to observe events on.

Global variables

The variables setup at the top (i.e. x, y, znacznik, as well as all the DOM references) are global to the window. It is wise to limit the scope to avoid namespace collisions. This can be achieved with various approaches, such as using an IIFE or wrapping the code in a DOM-loaded callback.

Checking for numbers

The calls to isNaN() will return either true or false so the == true can be omitted in conditional expressions. Similarly, many JavaScript developers use !isNaN() instead of isNaN() == false.

Input validation in wyniczek()

There is a line repeated many times in this function: alert("Thats wrong! use C and try again");. The code already checks to see if y is blank (i.e. if (y == "") alert("Co do **** karmazyna ?!?");) so a similar check could be made after that to ensure the input is valid. And if a return statement is added whenever the inputs are invalid, then there is no need to use the else block.

Using alert()

For this single page application it might be okay to use alert() but in a larger application it may be better to have a separate UI element to hold messages to the user. Read more about this topic in this answer by @blindman67.

Parsing integers with parseInt()

If you are going to use parseInt(), it is wise to specify the radix using the second parameter - unless you are using a unique number system like hexidecimal, octal, etc. then specify 10 for decimal numbers.

Always specify this parameter to eliminate reader confusion and to guarantee predictable behavior. Different implementations produce different results when a radix is not specified, usually defaulting the value to 10.1

Strict Equality

It is a best practice to use strict equality when comparing values, especially when you expect the types to be the same. The code above uses loose equality comparisons, like:

if (y == "")

With the strict equality operator, the type of the value of y would never need to be converted to a string in case it had a different type.

if (y === "")

Updated code

The code below utilizes suggestions from above to obtain the same functionality as the original code. It does use eval() to simplify the evaluation of the mathematic expressions but as you can see in the answers to this SO post using it is highly discouraged.

It also uses a few features added with the specification like Array.includes(), template literals, and the keywords const, and let so note the browser compatibility with those features if necessary.

document.addEventListener('DOMContentLoaded', function() {
  const resultArea = document.forms.formularzCalc.Wynik;
  let operation = "";
  let x;
  let y = "";
  const OPERATIONS = ["+", "-", "*", "/"];

  // operatory arytmetyczne i przypisywanie wartości resultArea do x
  function setOperation(value) {
    x = resultArea.value;
    resultArea.value = "";
    operation = value;
  }

  // przypisywanie wartości resultArea do y i obliczanie
  function setResult() {
    y = resultArea.value;
    if (y === "") {
      alert("Co do **** karmazyna ?!?");
      return;
    }
    if (y === 0 && operation === "/" || isNaN(x) || isNaN(y)) {
      alert("Thats wrong! use C and try again");
      return;
    }
    if (OPERATIONS.includes(operation)) {
      resultArea.value = eval(`${parseInt(x, 10)} ${operation} ${parseInt(y, 10)}`);
    }
  }
  document.addEventListener('click', function(clickEvent) {
    if (clickEvent.target.tagName.toLowerCase() === 'input') {
      const value = clickEvent.target.value;
      const intValue = parseInt(value, 10);
      if (!isNaN(intValue)) {
        if (resultArea.value != "" || intValue) { //LiczbaX(), LiczbaY()
          resultArea.value += intValue
        }
      }
      if (OPERATIONS.includes(value)) {
        setOperation(value);
      }
      if (value === "=") {
        setResult();
      }
    }
  });
});
body {
  background-color: #fff0b7;
}


/* Edytowanie formularza */

form {
  width: 40%;
  margin: auto;
  text-align: center;
}

td input {
  width: 40px;
  height: 30px;
}

fieldset {
  border-color: #00998C;
  background-color: rgba(0, 0, 0, 0.05);
}

legend:first-letter {
  color: #DDcc67;
  font-size: 120%;
}

legend {
  background-color: #00998C;
  padding: 5px 15px;
  border-radius: 15px;
  font-size: 23px;
  font-weight: bolder;
  color: #eeeeee;
  font-family: "Arial Black";
  margin: 0 auto;
  text-shadow: 3px 3px 1px #333333;
}


/*  Edytowanie tabeli */

table {
  margin: auto;
}

th input {
  text-align: right;
}


/* reszta */

footer {
  width: 40%;
  background-color: #00998C;
  margin: 5px auto;
  text-align: center;
  border: 1px rgba(0, 0, 0, 0.30) solid;
  color: #EEEEEE;
  font-family: verdana;
  font-weight: bold;
  font-size: 80%;
  min-width: 250px;
  border-radius: 10px;
  padding: 3px 0px;
}

input[disabled] {
  box-shadow: 1px 1px 1px black;
}

img {
  position: absolute;
  left: 20px;
  top: 20px;
}
<form name="formularzCalc">
  <fieldset>
    <legend> Calc Upgraded version </legend>
    <table>
      <!-- Tabela do wyrównania zawartości kalkulatora -->
      <tr>
        <th colspan="4"><input type="text" name="Wynik" disabled></th>
        <!-- Użycie th w celu łatwiejszej edycji -->
      </tr>
      <tr>
        <td><input type="button" name="keySeven" value="7"></td>
        <td><input type="button" name="keyEight" value="8"></td>
        <td><input type="button" name="keyNine" value="9"></td>
        <td><input type="button" name="keyPlus" value="+"></td>
      </tr>
      <tr>
        <td><input type="button" name="keyFour" value="4"></td>
        <td><input type="button" name="keyFive" value="5"></td>
        <td><input type="button" name="keySix" value="6"></td>
        <td><input type="button" name="keyMinus" value="-"></td>
      </tr>
      <tr>
        <td><input type="button" name="keyOne" value="1"></td>
        <td><input type="button" name="keyTwo" value="2"></td>
        <td><input type="button" name="keyThree" value="3"></td>
        <td><input type="button" name="keyMulti" value="*"></td>
      </tr>
      <tr>
        <td><input type="reset" name="keyCancel" value="C"></td>
        <td><input type="button" name="keyZero" value="0"></td>
        <td><input type="button" name="keyResult" value="="></td>
        <td><input type="button" name="keyDivide" value="/"></td>
      </tr>
    </table>
  </fieldset>
</form>
<footer>
  The new Calc made by Hyakkimaru <sup>&copy;</sup> Wszelkie prawa zastrzeżone
</footer>


1https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#Parameters

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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