Phoenix - C22 - Tram Hoang #36
Conversation
| @@ -120,8 +120,9 @@ describe("Adagrams", () => { | |||
| }); | |||
|
|
|||
| it("returns a score of 0 if given an empty input", () => { | |||
| throw "Complete test"; | |||
| expect(scoreWord("")).toBe(0); | |||
There was a problem hiding this comment.
We could also use the expectScores helper as
expectScores({
'': 0,
});| @@ -133,7 +134,7 @@ describe("Adagrams", () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe.skip("highestScoreFrom", () => { | |||
| describe("highestScoreFrom", () => { | |||
| @@ -145,7 +146,7 @@ describe("Adagrams", () => { | |||
| const words = ["XXX", "XXXX", "X", "XX"]; | |||
| const correct = { word: "XXXX", score: scoreWord("XXXX") }; | |||
|
|
|||
| throw "Complete test by adding an assertion"; | |||
| expect(highestScoreFrom(words)).toEqual(correct); | |||
| @@ -1,15 +1,116 @@ | |||
| // import { LETTER_POOL } from '../test/adagrams.test.js'; | |||
There was a problem hiding this comment.
Good call to exclude this. Our implementation logic should never depend on anything from the tests.
| while (lettersInHand.length < 10) { | ||
| const index = Math.floor(Math.random() * letterList.length); | ||
| lettersInHand.push(letterList[index]); | ||
| letterList.splice(index, 1); |
There was a problem hiding this comment.
Much like pop/remove in python, splice will need to shift values forward to close the hole left by the removed item. To avoid the internal loop this requires, we can first swap it with value to the end of the pool, then pop it. Strictly speaking, it's not even necessary to remove the value from the list, so lang as we adjusted the value we used as the list length to reflect the size of the available portion of the pool.
| for (const letter of word.toUpperCase()) { | ||
| totalScores += scoreChart[letter]; | ||
| } |
There was a problem hiding this comment.
We'll often see summing up a result handled with the Array.reduce method.
totalScores = word. toUpperCase().split('').reduce((acc, letter) => acc + scoreChart[letter], 0)I tend to find the explicit loop version you used to be more readable, but reduce is pretty popular among JS afficionados.
| if (word.length > 6) { | ||
| totalScores += 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 LENGTH_BONUS = 8;then
if (word.length >= MIN_BONUS_LENGTH) {
totalScores += LENGTH_BONUS;
}| let bestWord = ''; | ||
| let bestScore = 0; | ||
|
|
||
| for (let word of words) { |
There was a problem hiding this comment.
Prefer const
for (const word of words) {| let bestScore = 0; | ||
|
|
||
| for (let word of words) { | ||
| let score = scoreWord(word); |
There was a problem hiding this comment.
Prefer const
const score = scoreWord(word);| } else if (score === bestScore) { | ||
| if (word.length === 10 && bestWord.length !== 10) { | ||
| bestWord = word; | ||
| } else if (word.length < bestWord.length && bestWord.length !== 10) { | ||
| bestWord = word; | ||
| } | ||
| } |
There was a problem hiding this comment.
Notice that both of the nested conditions requires that the winning word not have a length of 10. We could move those checks up to the outer condition, and simplify the nested checks.
else if (score === bestScore && bestWord.length !== 10) {
if (word.length === 10) {
bestWord = word;
} else if (word.length < bestWord.length) {
bestWord = word;
}
}We can also observe that the body of each of those checks is identical, so they could be combined into a single check. We could even squish them all together in that outer else, but that might be getting hard to read.
All tests of 4 waves passed. I also demo played and it worked.