Basic Calculator

Question

I'm in a beginner JavaScript class and I feel like I have a pretty good grasp on all the intro material. All of my homework assignments work but they seem longer than they have to be, especially my most recent one. I'm not asking for answers to my homework as it is already complete (you can see the code works below) but I was seeing if anyone can give me some ideas for making my code more efficient.

Please don't provide me with new code and say paste this in as I really would like to think about it and learn. Maybe I haven't learned enough yet, but this looks pretty ugly to me from what I've seen online and I also repeat code... The only thing I'm required to do is build this with an object and there can only be one answer displayed at a time. I can't use jQuery or any other libraries.

var obj = {
     num1 : document.getElementById('num1'),
     num2 : document.getElementById('num2'),
     add : document.getElementById("add"),
     sub : document.getElementById("sub"),
     mult : document.getElementById("mult"),
     div : document.getElementById("div"),
     result : document.getElementById("result"),
     init : function() {
         document.getElementById("calculate").onclick = obj.calc;
     },
     calc : function() {
         var num1 = parseFloat(obj.num1.value);
         var num2 = parseFloat(obj.num2.value);
         if(isNaN(num1) || isNaN(num2)) {
             alert("You must enter a number for First Number and Second Number.");
         }
         else if (num2 === 0) {
              alert("You cannot divide by zero"); 
         }
         else {
             if (obj.add.checked === true) {
                 var result = num1 + num2;
             }
             else if (obj.sub.checked === true) {
                 var result = num1 - num2;
             }
             else if (obj.mult.checked === true) {
                 var result = num1 * num2;
             }
             else if (obj.div.checked === true) {
                 var result = num1 / num2;
             }
             else {
                 alert("Choose an operator")
             } 

            if (obj.result.firstChild){
                 var para = document.getElementById("para");
                 console.log(para);
                 para.parentNode.removeChild(para);
                 var p = document.createElement("p");
                 p.setAttribute("id", "para");
                 p.appendChild(document.createTextNode("The answer is " + result));
                 obj.result.appendChild(p);
             }
             else {
                 var p = document.createElement("p");
                 p.setAttribute("id", "para");
                 p.appendChild(document.createTextNode("The answer is " + result));
                 obj.result.appendChild(p);
             }
         }
     }
 }  

Answer 1

rather than query each radio to see if it is checked and then run the arithmetic based upon that, I would probably store the arithmetic as a closure in your obj, where the key is the value of the checked input and then run document.querySelector to find the checked input, something like this:

var obj = {
 num1 : document.getElementById('num1'),
 num2 : document.getElementById('num2'),
 add  : function(n1, n2) { return n1 + n2; },
 sub  : function(n1, n2) { return n1 - n2; },
 mult : function(n1, n2) { return n1 * n2; },
 div  : function(n1, n2) { return n1 / n2; },
 result : document.getElementById("result"),
 init : function() {
     document.getElementById("calculate").onclick = obj.calc;
 },
 calc : function() {
     var num1 = parseFloat(obj.num1.value),
         num2 = parseFloat(obj.num2.value),
         operation, result;

     if(isNaN(num1) || isNaN(num2)) {
        return alert("You must enter a number for First Number and Second Number.");
     }
     else if (num2 === 0) {
        return alert("You cannot divide by zero"); 
     }

    operation = document.querySelector('input[type="radio"]:checked').value;

    result = obj[operation](num1, num2);

    obj.result.innerHTML = '';           

    p = document.createElement("p");
    p.setAttribute("id", "para");
    p.appendChild(document.createTextNode("The answer is " + result));
    obj.result.appendChild(p);
 }
 }

obj.init();  

This saves you from having a really long series of if / else checking for the checked input. I would also remove the logic of checking whether you have already created a P and added it to the result div, instead i'd just set the result divs innerHTML to '' each time, and add the P tag again, as the end result is the same.

Answer 2

I like Keir Lavelle's answer, but I would modify it like this:

operation = document.querySelector('input[name="operation"]:checked').value;

if(isNaN(num1) || isNaN(num2)) {
    return alert("You must enter a number for First Number and Second Number.");
} 
else if (num2 === 0 && operation == div) {
    return alert("You cannot divide by zero"); 
}

1. move code for finding operation above the check for divide-by-zero
2. change it to get radio list by name, not type (to avoid conflicts with new radio lists)
3. in check for divide-by-zero, also check if the operator is division. With any other operator, it is okay to have zero for num2

Answer 3

Welcome to CR, from a once over:

• You are warning about division by 0, even if I did not select 'Divide'
• You can consider using truthy evaluations to shorten code: (obj.add.checked === true) -> (obj.add.checked)
• I would not go for the literal object notation to avoid having to type ( and read ) obj. every single time
• Instead of using onclick = obj.calc;, consider using addEventListener

• It would be smart to already have a para node as part of the HTML, and then simply set the value of that para node, this

      if (obj.result.firstChild){
           var para = document.getElementById("para");
           console.log(para);
           para.parentNode.removeChild(para);
           var p = document.createElement("p");
           p.setAttribute("id", "para");
           p.appendChild(document.createTextNode("The answer is " + result));
           obj.result.appendChild(p);
       }
       else {
           var p = document.createElement("p");
           p.setAttribute("id", "para");
           p.appendChild(document.createTextNode("The answer is " + result));
           obj.result.appendChild(p);
       }
  

  would become:

  obj.para.innerText = "The answer is " + result;
  

  assuming that on top you would declare

   para : document.getElementById("para"),
  

• obj, as names go, is pretty banal, I am sure you can do better

Answer 4

Contrary to common belief, division by zero is not always disastrous. In fact, the IEEE 754 floating point standard specifies that dividing by zero produces +∞ or -∞. JavaScript treats numbers as IEEE double-precision floating point, so you get the following behaviour for free:

> 3 / 0
Infinity
> 3 / -0
-Infinity
> 1 / 0 - 1
Infinity

Your calculator would be simpler and more useful if you don't check for division by zero.