WeiQ- Sphinx #29
WeiQ- Sphinx #29
Conversation
| @@ -1,15 +1,114 @@ | |||
| const LETTER_POOL = { | |||
There was a problem hiding this comment.
Nice job using capital letters to name your constant variable!
| const LetterPoolList = []; | ||
| for (const [letter, count] of Object.entries(LETTER_POOL)) { | ||
| for (let i = 0; i < count; i++) { | ||
| LetterPoolList.push(letter); | ||
| } | ||
| } |
There was a problem hiding this comment.
Prefer this logic be put in a helper function that you can call in drawLetters
| LetterPoolList.push(letter); | ||
| } | ||
| } | ||
| const totalLetterCount = LetterPoolList.length; |
There was a problem hiding this comment.
| const totalLetterCount = LetterPoolList.length; | |
| const TOTAL_LETTER_COUNT = LetterPoolList.length; |
| const lastIndex = totalLetterCount - i - 1; | ||
| [LetterPoolList[randomIndex], LetterPoolList[lastIndex]] = [LetterPoolList[lastIndex], LetterPoolList[randomIndex]] |
There was a problem hiding this comment.
😎 nice work swapping the randomly drawn letter to the end of the list! This a constant time operation which beats an approach that uses splice to remove drawn letters.
| const letterFreMap = {}; | ||
| for (const letter of lettersInHand) { | ||
| if (!(letter in letterFreMap)) { | ||
| letterFreMap[letter] = 1; | ||
| } else { | ||
| letterFreMap[letter] += 1; | ||
| } |
There was a problem hiding this comment.
👍 I like the frequency map here so you can leverage the constant time look up.
| word = word.toUpperCase(); | ||
| for (let char of word) { |
There was a problem hiding this comment.
We should use const for the looping variable char since this variable's value doesn't change in the loop.
Can also write lines 88-89 in one line:
| word = word.toUpperCase(); | |
| for (let char of word) { | |
| for (const char of word.toUpperCase()) { |
| score += SCORE_CHART[char]; | ||
| } | ||
|
|
||
| if ([7, 8, 9, 10].includes(word.length)) { |
There was a problem hiding this comment.
While the time complexity of using includes in this method ends up being constant because you're always only checking that a word's length is in a list of four integers, I'd prefer this to be written like
| if ([7, 8, 9, 10].includes(word.length)) { | |
| if (7 <= word.length <= 10)) { |
Writing the check this way feels like it conveys that you're checking the length is between x and y value.
| for (const word of words) { | ||
| if (scoreWord(word) > currentMax) { | ||
| currentMax = scoreWord(word); | ||
| currentWinWord = word; | ||
| } else if (scoreWord(word) === currentMax && currentWinWord.length != 10) { |
There was a problem hiding this comment.
Between lines 104-107, you invoke the scoreWord method three times. Prefer you calculate the score of the current word once and save it to a variable.
| for (const word of words) { | |
| if (scoreWord(word) > currentMax) { | |
| currentMax = scoreWord(word); | |
| currentWinWord = word; | |
| } else if (scoreWord(word) === currentMax && currentWinWord.length != 10) { | |
| for (const word of words) { | |
| const wordScore = scoreWord(word); | |
| if (wordScore > currentMax) { | |
| currentMax = wordScore; | |
| currentWinWord = word; | |
| } else if (wordScore === currentMax && currentWinWord.length != 10) { |
| currentMax = scoreWord(word); | ||
| currentWinWord = word; | ||
| } else if (scoreWord(word) === currentMax && currentWinWord.length != 10) { | ||
| if(word.length < currentWinWord.length || word.length === 10) |
There was a problem hiding this comment.
| if(word.length < currentWinWord.length || word.length === 10) | |
| if (word.length < currentWinWord.length || word.length === 10) |
| @@ -120,7 +120,9 @@ describe("Adagrams", () => { | |||
| }); | |||
|
|
|||
| it("returns a score of 0 if given an empty input", () => { | |||
| throw "Complete test"; | |||
| expectScores({ | |||
| '':0 | |||
No description provided.