Kunzite--Tuminello, Ann #54
Conversation
| @@ -1,15 +1,160 @@ | |||
| export const drawLetters = () => { | |||
| // Implement this method for wave 1 | |||
| const letterPool = { | |||
There was a problem hiding this comment.
Consider defining letterPool outside the function to focus on the code. Since the logic modifies the letter counts, we would need to make a copy of the pool data so as not to clobber the data for subsequent calls.
| hand.push(letter); | ||
| letterPool[letter] -= 1; | ||
| } else { | ||
| i -= 1; |
There was a problem hiding this comment.
👀 If our for loop requires manually manipulating the index variable, then maybe a while loop is a better match for the logic. While we can do this, it's usually confusing and easy to overlook.
The driver for this here is that the main loop might not successfully pick a letter on each loop, meaning we don't know how many times the loop should run. We can capture that by rewriting as a loop that runs while the hand length is less than the needed number of letters.
| let hand = []; | ||
|
|
||
| for (let i = 0; i < 10; i++) { | ||
| let letter = Object.keys(letterPool)[Math.floor(Math.random() * 26)]; |
There was a problem hiding this comment.
👀Object.keys must iterate over the data each time it's called. Prefer performing this call once outside the loop, and use its result each time through the loop to significantly reduce the iterations required.
This logic selects letters with equal weighting. You have logic to reject invalid letters (letters we've already used up), but be aware that this will tend to select hands with an overrepresentation of "hard" letters, which would make it harder for the player to form words.
There was a problem hiding this comment.
👀 Prefer to avoid "magic numbers" like 26. Instead, stored the result of Object.keys in a variable, and then we could use its length!
| const result = scoreWord(word); | ||
|
|
||
| // Assert | ||
| expect(result).toBe(0); |
There was a problem hiding this comment.
👍 Note that we could have used the same helper as the other score tests.
expectScores({
"": 0,
});| it('returns null for an empty array', () => { | ||
| const words = []; | ||
|
|
||
| expect(highestScoreFrom(words)).toBeNull(); | ||
| }); | ||
|
|
||
| it('returns the only word in the array if it is the highest-scoring word', () => { | ||
| const words = ['APPLE']; | ||
| const correct = { word: 'APPLE', score: scoreWord('APPLE') }; | ||
|
|
||
| expect(highestScoreFrom(words)).toEqual(correct); | ||
| }); |
There was a problem hiding this comment.
👍 Nice custom tests. The specification didn't describe the empty list behavior, but returning null is one possibility. We might consider throwing an error, though JS is a little less error-happy than is Python.
|
|
||
| export const scoreWord = (word) => { | ||
| const scoreChart = { | ||
| 'A': 1, 'E': 1, 'I': 1, 'O': 1, 'U': 1, 'L': 1, 'N': 1, 'R': 1, 'S': 1, 'T': 1, |
There was a problem hiding this comment.
👀 Prefer to list this out alphabetically. It's hard for a human reader to be able to tell whether all the letters are there.
There was a problem hiding this comment.
Consider moving the score chart out of the function to put the focus on the logic.
| if (word === ''){ | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
We can do without this special case.
| for (let i = 0; i < wordUpper.length; i++) { | ||
| const letter = wordUpper[i]; | ||
| score += scoreChart[letter] || 0; | ||
| } |
There was a problem hiding this comment.
👀 Prefer for/of
for (const letter of wordUpper) {
score += scoreChart[letter] || 0;
}| score += scoreChart[letter] || 0; | ||
| } | ||
|
|
||
| if ([7, 8, 9, 10].includes(word.length)) { |
There was a problem hiding this comment.
Consider sticking with relational operators (> <) rather than using list membership
| if (wordScore > highestScore) { | ||
| highestScore = wordScore; | ||
| winningWord = word; | ||
| } else if (wordScore === highestScore) { | ||
| if (word.length === 10 && winningWord.length !== 10) { | ||
| winningWord = word; | ||
| } else if (word.length < winningWord.length && winningWord.length !== 10) { | ||
| winningWord = word; | ||
| } |
There was a problem hiding this comment.
👍 Nice capturing of all the cases we need to handle ties. For a more javascript-esque approach, rather than a plain for loop, consider using reduce!
No description provided.