Develop #55
Develop #55
Conversation
VeraVeraniego
requested review from
Gerald17,
blarzHernandez,
stevenheffner and
donovanhiland
| @@ -5,3 +5,171 @@ TO-DO: | |||
| - The calculator should be completely functional | |||
|
|
|||
| */ | |||
|
|
|||
| let a; | |||
There was a problem hiding this comment.
for next challenges, please try to use more descriptive variables names, because if you add more and more code later naming a variable like a or b would make the code confusing also I'd suggest to initialize the variables so that way we know which type of value you are expecting to assign...this problems get resolved with typescript later but please take it in count
| let fromEqual = false; | ||
|
|
||
| const display = document.getElementById("display"); | ||
| const zero = document.getElementById("zero"); |
There was a problem hiding this comment.
Do you think is a better way to get the values? I'd like you to think how you can avoid adding a selector for each number...let's say for example that you have 100 numbers? how would you that? is there another selector you can use? maybe one selector for all numbers and another for operations? try to think of a better way to avoid duplicating this
|
|
||
| const result = () => { | ||
| if(!display.textContent) return; | ||
| if(fromEqual) { |
There was a problem hiding this comment.
for readability, add a blank line after each if statement and before each return we can solve some of this issues with prettier and eslint rules
| case "/": | ||
| if(Number(b) == 0){ | ||
| display.textContent ="Error"; | ||
| setTimeout(() => { |
There was a problem hiding this comment.
why do we need a setTimeout here? it seems like you are just cleaning the values but to me if you get an error you can immediately clean the values and just keep the error message with the setTimeout, what do you think? is that what you needed?
| fromEqual = false; | ||
| return; | ||
| } | ||
| if(!a) { |
There was a problem hiding this comment.
is there a way you are checking a or b are always numbers?
| prevResult(); | ||
| operation = "+"; | ||
| if(prev) { | ||
| prev = false; |
There was a problem hiding this comment.
can you give more context about what is the purpose of the prev variable?
| const equals = document.getElementById("equals"); | ||
|
|
||
| const clean = () => { | ||
| display.textContent = ''; |
There was a problem hiding this comment.
just checking, did you read the differences between textContent, innerText, innerHtml?
|
|
||
| const hours = Math.floor(seconds / 3600); | ||
| const minutes = Math.floor((seconds - (hours * 3600)) / 60 ); | ||
| const second = seconds - (hours * 3600) - (minutes * 60); |
There was a problem hiding this comment.
in this code I see you have 3600 and 60, to me this are your conversion factors, maybe it would be good to put this into a variable so you can easily change, reuse, etc
| const minutes = Math.floor((seconds - (hours * 3600)) / 60 ); | ||
| const second = seconds - (hours * 3600) - (minutes * 60); | ||
|
|
||
| const time = hours.toString().padStart(2,'0') + ':' + minutes.toString().padStart(2,'0') + ':' + second.toString().padStart(2,'0'); |
There was a problem hiding this comment.
maybe a new function here could be good, also have you use template strings before?
| if( isNaN(index) || index < 0 ) return null; | ||
|
|
||
| const newIndex = index % COUNTRY_NAMES.length; | ||
| const newArray = [ ...COUNTRY_NAMES, ...COUNTRY_NAMES ]; |
There was a problem hiding this comment.
can you think in a way of doing this without concatenating the same array twice?
No description provided.