Pullrequest#1
Conversation
AmeliaIsCoding
left a comment
AmeliaIsCoding
left a comment
There was a problem hiding this comment.
Looks great! A few comments on things to change :)
| const upperValue: string | null = prompt("What value would you like FizzBuzz to go up to? "); | ||
| console.log(`We go up to ${upperValue}`); | ||
|
|
||
| function divisibleBy(num: number, divisor: number): boolean { return num % divisor == 0 }; |
There was a problem hiding this comment.
Move this function declaration to a different file and export it to be used in this file. You could call the new file helpers.ts or something like that and you should probably put it in a folder called src
The function makes sense on its own so it belongs in its own file :)
| const upperValue: string | null = prompt("What value would you like FizzBuzz to go up to? "); | ||
| console.log(`We go up to ${upperValue}`); | ||
|
|
||
| function divisibleBy(num: number, divisor: number): boolean { return num % divisor == 0 }; |
There was a problem hiding this comment.
I know num and divisor are both of the same type but people tend to use triple equals instead of double equals anyway so I would change the double equals to a triple equals
|
|
||
| function divisibleBy(num: number, divisor: number): boolean { return num % divisor == 0 }; | ||
|
|
||
| const key: {[index: number]: Function } = { |
There was a problem hiding this comment.
what does key mean? can we name this something more informative maybe?
There was a problem hiding this comment.
Also can we be more specific with the type of function? it appears to always be (answer: string[]) => string[]
| answer.push('Bang'); | ||
| return answer; | ||
| } else { | ||
| return ['Bang']; |
There was a problem hiding this comment.
Why don't we want to push "Bang" in this else clause?
| return ['Bang']; | ||
| } | ||
| }, | ||
| 11: (answer: string[]) => answer = ['Bong'], |
There was a problem hiding this comment.
could we change this to 11: (answer: string[]) => ['Bong'] ?
|
|
||
| } | ||
|
|
||
| const fizzBuzz = (upperValue: string | null) => { |
There was a problem hiding this comment.
should we explicitly treat the case where upperValue is null? how about console logging an error message and returning from the function?
| } | ||
|
|
||
| const fizzBuzz = (upperValue: string | null) => { | ||
| for (let n = 0; n <= Number(upperValue); n++) { |
There was a problem hiding this comment.
if we explicitly treat the case where upperValue is null, we don't need to convert upperValue to a number here
| let answer: string[] = []; | ||
|
|
||
| for (const num of Object.keys(key)) { | ||
| if (divisibleBy(n, Number(num))) { |
There was a problem hiding this comment.
is this type conversion needed? what type does typescript think num is?
| } | ||
| } | ||
|
|
||
| if (answer.length == 0) { answer.push(String(n))} |
There was a problem hiding this comment.
instead of using String(n) we could use n.toString(), I think this is more conventional
There was a problem hiding this comment.
Maybe you meant to delete this file before making the pull request! :)
2 participants
No description provided.