Almog Lavi - Fixed#11
Conversation
| @@ -0,0 +1,73 @@ | |||
|
|
|||
| let operator = undefined; | |||
There was a problem hiding this comment.
undefined is the default value of let variables, you could simply let operator
|
|
||
| number.forEach((num) => { | ||
| num.addEventListener('click', function() { | ||
| let digit = parseInt(num.textContent); |
There was a problem hiding this comment.
This could be NAN so you should add a check for it
| number.forEach((num) => { | ||
| num.addEventListener('click', function() { | ||
| let digit = parseInt(num.textContent); | ||
| if (operator===undefined) |
There was a problem hiding this comment.
You could also write if(!operator)
| let digit = parseInt(num.textContent); | ||
| if (operator===undefined) | ||
| { | ||
| x=x*10+digit; |
There was a problem hiding this comment.
Try to save 10 into a constant vriable, random numbers in the code are called magic numbers, very confusing
There was a problem hiding this comment.
didn't understand what's the confusing part
| { | ||
| x=x*10+digit; | ||
| display.textContent = x; | ||
| return; |
| }) | ||
|
|
||
| multiply.addEventListener('click', function() { | ||
| operator = '*' |
There was a problem hiding this comment.
Can you think of a way to only write 1 event listeners for all of the operators?
|
|
||
| clear.addEventListener('click',function(){ | ||
| resetAll(); | ||
| display.textContent=''; |
There was a problem hiding this comment.
Why not insert this line into resetAll function?
From the name the function should reset all, the display as well
|
|
||
| equal.addEventListener('click',function() { | ||
| let result; | ||
| if (operator === '+') |
There was a problem hiding this comment.
Save your strings into constant variable or even an object for easy access
…for the is statements.
3 participants
No description provided.