feedback on my calc :)

[javascriptnewbie]

Alright, yesterday I made a thread on how arrays could be used in real situation I thought about it and this is what I came up with I used loop in an array to add tags dynamically on page.. anyways gimme your opinion on it : )

https://dl.dropboxusercontent.com/u/28490184/calculator/index.html

[noahgelman]

A couple tips. A couple bugs.

Variables should be var button = not just button =. This makes your code cleaner and easier to read

In the if/else statement (firstNumber1=="") && ( secondNumber1 =="") it still calculates when one of them is empty. Everything comes up NaN. You should use the OR sign to prevent calculating till both are filled.

Instead of making the rest of it sit inside the if/else statement, you can just add a return inside the if statement, like so:

if((firstNumber1=="") || ( secondNumber1 =="")){
  alert("Please fill in valid numbers");
  return;
}

This will stop the rest of the code from executing if the if statement equals true. This is less code, less indenting, less nested code, which equals healthier cleaner code.

“remainder” didn’t make a lot of sense till I looked at the code.

In your For loop, i<=4 has 2 issues. 1 is that since its iterating over i, it can never be equal to 4. you can just use <. 2nd is what happens when you change the length of the box and sign arrays? You’ll have to update the 4. This can be harder to remember to do in every little place when you start building larger programs. Instead use i<signs.length. Now you never have to update it no matter how big or small it gets.

You’re only getting whole numbers. Numbers with decimal points do not show accurate results

Lastly, instead of having to arrays, you should just make it into an Object Literal (you can google that) like so:

var equations = {
    'addition' : 'When added: ',
    'subtraction' : 'When subtracted: ',
    'division' : 'When divided: ',
    'multiplication' : 'When Multiplied: ',
    'remainder' : 'Remainder:'
}

And then you can get their values like so:

for (var text in equations ) {
    alert(text);
    alert(equations [text]);
}

Combining this stuff makes it easier to work with. I hope that helps.

[noahgelman]

Let me know if you have any questions

[javascriptnewbie]

Hey man, understood everything clearly. I really appreciate your feedback and tips : ) – will definitely let you know : )

[__]

Variables should be var button = not just button =. This makes your code cleaner and easier to read

This does make things “cleaner,” but it isn’t about readability: it directly affects the variable’s scope.

In the if/else statement (firstNumber1==””) && ( secondNumber1 ==””) it still calculates when one of them is empty. Everything comes up NaN. You should use the OR sign to prevent calculating till both are filled.

Instead of making the rest of it sit inside the if/else statement, you can just add a return inside the if statement …This will stop the rest of the code from executing if the if statement equals true. This is less code, less indenting, less nested code, which equals healthier cleaner code.

Agreed, though I would recommend testing that the params are actually numbers, rather than testing if they are empty strings.

if( isNaN( firstNumber ) || isNaN( secondNumber ) ){
    // abort
}

I’m using firstNumber instead of firstNumber1* in this example because you got it from parseInt: meaning it will be either a valid (math-able) number, or it will be NaN (Not A Number, which is what isNaN checks).

* Why do you create firstNumber1 and secondNumber1 in the first place? You’re using them to check the validity of _different vars (which, as demonstrated, is misleading), and I don’t see you use them anywhere else…?_

“remainder” didn’t make a lot of sense till I looked at the code.

Why don’t you call it the “modulus” (since that’s what it is)?

You’re only getting whole numbers. Numbers with decimal points do not show accurate results

…because you used parseInt, which forces the numbers to be Integers: the decimal part is dropped. Is this what you intended? If not, use parseFloat. (But be aware of the inherent problems with floating-point mathematics.)

[Author]

Posts