Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
             }
         }
     }
 }  
share|improve this question

4 Answers 4

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.

share|improve this answer
    
That's exactly the thought process I was using, but I didn't know how to go about it. Haven't used querySelector yet. Clearing the div makes a lot more sense. Thanks for the response! –  Vince D'Onofrio Apr 2 at 15:24

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
share|improve this answer
1  
What would be the reason for changing to name? I don't think it would matter in this specific case. I think I am answering my own question, but is it in the case that there are multiple radio buttons on a page and you only want to apply this code to the operations buttons? Thanks for the divide catch, I completely overlooked the fact that 0 didn't work for other operations. –  Vince D'Onofrio Apr 2 at 15:32
1  
Yes, that's exactly why. You're right that it wouldn't matter in this case. But once you add another radio button list to the page (for a different thing altogether), suddenly things won't work. –  Will Newton Apr 3 at 1:17

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

share|improve this answer
    
Thanks for the response! I didn't even think to use 0 if other operations were selected, thanks for that catch. I wasn't very clear on my assignment details. obj has to be the object name... I know this can be a lot better. I also can't edit the HTML file that was provided and have to use literal object notation. I haven't used addEventListener before so I'm going to look into that now. Thanks again! –  Vince D'Onofrio Apr 2 at 15:19

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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