C22 Phenix Luqi Xie #33
Conversation
| Z: 1, | ||
| }; | ||
|
|
||
| const SCORE_CHART = { |
There was a problem hiding this comment.
The order of the letters aren't important to JS, but we can make it a little easier to double check that the scores are all present and accounted for if we wrote this in alphabetical order.
const SCORE_CHART = {
A: 1,
B: 3,
C: 3,
...
};| [letterList[letterIndex], letterList[lastPos]] = [letterList[lastPos], letterList[letterIndex]]; | ||
| drawLetters.push(letterList.pop()); |
There was a problem hiding this comment.
Nice approach of swapping the picked letter to the end, which then allows us to pop in constant time. This makes the complexity dependent only upon the size of the hand.
| for (const letter of input.toUpperCase()) { | ||
| if (!letterFrequencyMap[letter] || letterFrequencyMap[letter] === 0) { | ||
| return false; | ||
| } else { | ||
| letterFrequencyMap[letter] -= 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Nice use of a frequency map to check the validity of the owrd against th hand. We can build the map in a single iteration over the hand, then validate the word in a single iteration over the word, resulting in time complexity O(m + n), where m is the size of the hand and n is the size of the word.
| if (!letterFrequencyMap[letter] || letterFrequencyMap[letter] === 0) { | ||
| return false; | ||
| } else { | ||
| letterFrequencyMap[letter] -= 1; |
There was a problem hiding this comment.
Even though JS doesn't use indentation to control the flow of logic, we should still make sure to follow good indentation practice to help with readability. So we'd want to indent this line.
| if (score > highestScore) { | ||
| highestScore = score; | ||
| bestWord = word; | ||
| } else if (score === highestScore && bestWord.length !== 10) { |
There was a problem hiding this comment.
Each of the nested cases would only apply if the best word wasn't already of length 10, so moving that up to this condition helps simplify the nested conditions.
| expectScores({ | ||
| "": 0, | ||
| }); |
| @@ -133,7 +136,7 @@ describe("Adagrams", () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe.skip("highestScoreFrom", () => { | |||
| describe("highestScoreFrom", () => { | |||
| @@ -145,7 +148,8 @@ describe("Adagrams", () => { | |||
| const words = ["XXX", "XXXX", "X", "XX"]; | |||
| const correct = { word: "XXXX", score: scoreWord("XXXX") }; | |||
|
|
|||
| throw "Complete test by adding an assertion"; | |||
| // throw "Complete test by adding an assertion"; | |||
| expect(highestScoreFrom(words)).toEqual(correct); | |||
| let totalScore = 0; | ||
| for (const letter of word.toUpperCase()) { | ||
| totalScore += SCORE_CHART[letter]; | ||
| } |
There was a problem hiding this comment.
Great use of a score lookup table to keep this simple. We could also use Array.reduce to perform this summation.
let totalScore = word.toUpperCase().split('').reduce((acc, letter) => acc + SCORE_CHART[letter], 0);Personally, I find the loop approach you used more readable, but the reduce approach is also popular with JS programmers.
Co-authored-by: Ansel Rognlie <[email protected]>
No description provided.