New solution#25
Conversation
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Hey i started to review this and notices that this is the same as your last one, please refer to the prev review (you opened 2 prs for the same changes i think)
| @@ -0,0 +1,74 @@ | |||
| let display=document.querySelector('.display') | |||
| let x,y; | |||
There was a problem hiding this comment.
Can you think of more meaningful names? x and y are not really indicating what those variables are
| let multiply = document.getElementById('multiply') | ||
| let divide = document.getElementById('divide') | ||
|
|
||
| plus.addEventListener('click', function(){ |
There was a problem hiding this comment.
If you can target all of the operators, you will be able to use 1 function for all of them instead of 4 like now
| display.textContent='+'; | ||
| }) | ||
| subtract.addEventListener('click', function(){ | ||
| operator='-'; |
There was a problem hiding this comment.
Hardcoded strings should be saved inside constants variables
| display.textContent='/'; | ||
| }) | ||
|
|
||
| let equal = document.querySelector('#equal') |
There was a problem hiding this comment.
Save the variables on top of the file
| let num = document.querySelectorAll('.number').forEach(btn=>{ | ||
| btn.addEventListener('click', function(){ | ||
| if(!operator){ | ||
| x=Number(btn.textContent); |
There was a problem hiding this comment.
This could be NAN add a check for it
| x=Number(btn.textContent); | ||
| display.textContent=btn.textContent; | ||
| } | ||
| else{ |
There was a problem hiding this comment.
With early return statement above you dont need this else
| result=x+y; | ||
| display.textContent=result; | ||
| } | ||
| if(operator=='-'){ |
There was a problem hiding this comment.
Add else if to not check all of the ifs every time
|
|
||
| }) | ||
|
|
||
| let clear = document.getElementById('clear') |
There was a problem hiding this comment.
Save this variable on top of this file
2 participants
No description provided.