Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good overall! Please review my comments, and let me know if you have any questions.
|
|
||
| it("returns a score of 0 if given an empty input", () => { | ||
| throw "Complete test"; | ||
| expectScores({ |
| }); | ||
|
|
||
| describe.skip("highestScoreFrom", () => { | ||
| describe("highestScoreFrom", () => { |
| const correct = { word: "XXXX", score: scoreWord("XXXX") }; | ||
|
|
||
| throw "Complete test by adding an assertion"; | ||
| expect(highestScoreFrom(words)).toEqual(correct); |
| const SCORE_CHART = { | ||
| 1: ['A', 'E', 'I', 'O', 'U', 'L', 'N', 'R', 'S', 'T'], | ||
| 2: ['D', 'G'], | ||
| 3: ['B', 'C', 'M', 'P'], | ||
| 4: ['F', 'H', 'V', 'W', 'Y'], | ||
| 5: ['K'], | ||
| 8: ['J', 'X'], | ||
| 10: ['Q', 'Z'], | ||
| }; |
There was a problem hiding this comment.
As with the python version of this code, storing the score chart as score → list of letters is suboptimal given that when we are scoring a word, the thing that we have is the letter and the thing we are trying to lookup is the score. We should structure our key-value pairs so the thing that we have is the key, and the thing we want to lookup is the value. Otherwise, we need to iterate through the object to find the data relationship, making it no better than a list. If we are using a dict (object) then we should use it in a way that makes use of its key feature: efficient value lookup by key.
| let idx = 0; | ||
| for (const [key, value] of Object.entries(LETTER_POOL)) { | ||
| copyPool[idx] = { [key]: value }; | ||
| idx++; | ||
| } |
There was a problem hiding this comment.
The data structure design here is cumbersome, making it difficult to access the single pair in each nested object without resorting to iteration. To store the pair in a way that's easier to access, either use a list [key, value], which can be access via position, or named keys that we can use to access the pieces by name { letter: key, count: value }.
There was a problem hiding this comment.
Building a lookup table like this, to have a letter randomly chosen later (from position 0 - 25) does not properly model the distribution of tiles that would be available if we were actually drawing tiles from a pool that contained duplicates. The "rare" letters will have a much higher chance of being chosen, and the chance does not change as letters are picked.
Think of a way to write the picking logic so that it mimics the way we would pick letters from a pile on a table. The pile contains as many copies of a letter as indicated in the LETTER_POOL data.
| if (upperWord.length >= 7 && upperWord.length <= 10) { | ||
| totalScore += 8; | ||
| } |
There was a problem hiding this comment.
Consider creating global constants to use instead of the magic numbers, to make this code more self-documenting.
const MIN_BONUS_LENGTH = 7;
const MAX_WORD_LENGTH = 10;
const LENGTH_BONUS = 8;then
if (upperWord.length >= MIN_BONUS_LENGTH && upperWord.length <= MAX_WORD_LENGTH) {
totalScore += LENGTH_BONUS;
}| for (const [score, letters] of Object.entries(SCORE_CHART)) { | ||
| if (letters.includes(letter)) { | ||
| totalScore += parseInt(score); | ||
| } | ||
| } |
There was a problem hiding this comment.
If the structure used for the SCORE_CHART were letter → score instead of score → list of letters, we would be able to
- Look up the score for a letter in constant time instead of linear
- Score the score as an actual number rather than a string (JS objects only have string keys!)
| let winningWord = null; | ||
| let highScore = 0; | ||
|
|
||
| for (let word of words) { |
There was a problem hiding this comment.
Perefer const
for (const word of words) {For for/of loops, const is almost always what we want. It's only the explicit for loop (for (init; cond; update) {}) where the variable typically needs to be a let variable.
| if ( | ||
| word.length === 10 && winningWord.length !== 10 || | ||
| word.length < winningWord.length && winningWord.length !== 10 || | ||
| word === winningWord |
There was a problem hiding this comment.
If word === winningWord then there's no need to update winningWord.
| word.length === 10 && winningWord.length !== 10 || | ||
| word.length < winningWord.length && winningWord.length !== 10 || |
There was a problem hiding this comment.
Consider grouping nested expressions = for clarity
(word.length === 10 && winningWord.length !== 10) ||
(word.length < winningWord.length && winningWord.length !== 10)Technically, the && operator has higher precedence than the || operator, but it can be difficult to keep track of that for complex conditions. Using parentheses makes our intent clear.
2 participants
No description provided.