Ocelots - Nadxelle Hernandez - #4
Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Looks good! I’ve added some suggestions, let me know if there's anything I can clarify ^_^
| @@ -1,15 +1,173 @@ | |||
| const SIZE_OF_DRAW = 10; | |||
There was a problem hiding this comment.
I like having a value for this that we could change in just one place if we wanted to update it.
| @@ -1,15 +1,173 @@ | |||
| const SIZE_OF_DRAW = 10; | |||
| const LETTER_VALUES = { | |||
There was a problem hiding this comment.
Since LETTER_VALUES is only accessed by the scoreWord function, it would be reasonable to place the object inside the function rather than as a constant. There are tradeoffs, the structure would clutter the function some, but it keeps the data as close as possible to where it's being used, and would mean other functions couldn't access it to accidentally alter the contents.
| Z: 10, | ||
| }; | ||
|
|
||
| const genPoolLetters = (poolRulesObj) => { |
There was a problem hiding this comment.
Nice helper to generate the letter distribution array to index into.
| const draw = []; | ||
| const poolLetters = genPoolLetters(poolRules); | ||
|
|
||
| while (draw.length < SIZE_OF_DRAW) { | ||
| let randNum = getRandomNumber(poolLetters.length); | ||
| draw.push(poolLetters[randNum]); | ||
| poolLetters.splice(randNum, 1); | ||
| } | ||
| return draw; |
There was a problem hiding this comment.
I love how nicely laid out this is and how easy it is to read with the helper functions.
| if (letterBank[letter] == undefined) { | ||
| return false; | ||
| } | ||
| if (letterBank[letter] == 0) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
If the line doesn't get too long, I suggest combining these into one if-statement so we can have one false return.
| if (!input || lettersInHand === []) { | ||
| return false; | ||
| } | ||
| const letterBank = createLettersInHandDict(lettersInHand); |
There was a problem hiding this comment.
Nice use of a frequency map =]
| if (letterBank[letter] == undefined) { | ||
| return false; | ||
| } | ||
| if (letterBank[letter] == 0) { |
There was a problem hiding this comment.
In javascript we should use === to make a strict comparison against value and type. This article gets into more of the distinctions between == and === which might be helpful: https://www.scaler.com/topics/javascript/difference-between-double-equals-and-triple-equals-in-javascript/
| for (let i = 0; i < word.length; ++i) { | ||
| let letter = word[i]; | ||
| if (LETTER_VALUES[letter] != undefined) { | ||
| // If its undefined means letter is a special character | ||
| points += LETTER_VALUES[letter]; | ||
| } | ||
| } |
There was a problem hiding this comment.
We could simplify this loop a little with a for...of loop:
for (const letter of word) {
if (LETTER_VALUES[letter] != undefined) {
// If its undefined means letter is a special character
points += LETTER_VALUES[letter];
}
}| } | ||
| let points = 0; | ||
| word = word.toUpperCase(); | ||
| for (let i = 0; i < word.length; ++i) { |
There was a problem hiding this comment.
If we won't be reassigning the loop variable in the body of a loop, I recommend using const over let to declare the variable.
2 participants
No description provided.