Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Nice work getting situated in JS. Remember to keep in mind where certain built-in function calls might be hiding a loop, and try to avoid using those within an outer loop (things like splice or includes). Even though the small size of the data in this means the absolute performance is still fast, we should keep the algorithmic complexity in mind. Imagine what would happen if we relaxed the data restrictions to use larger tile sets or hands.
Take a look at my feedback on drawLetters for an important point, but otherwise nicely done.
| it("does not draw a letter too many times", () => { | ||
| for (let i = 0; i < 1000; i++) { | ||
| const drawn = drawLetters(); | ||
| const letter_freq = {}; |
There was a problem hiding this comment.
We didn't really need to update this, but the linter was probably complaining about the case. 😂
|
|
||
| it("returns a score of 0 if given an empty input", () => { | ||
| throw "Complete test"; | ||
| expectScores({ |
|
|
||
| throw "Complete test by adding an assertion"; | ||
| // throw "Complete test by adding an assertion"; | ||
| expect(highestScoreFrom(words)).toEqual(correct); |
| } | ||
| } | ||
| while (hand.length < 10) { | ||
| hand.push(letters.pop(Math.floor(Math.random() * letters.length - 1))); |
There was a problem hiding this comment.
In JS, pop always pops from the end. We need to use something like splice to remove from some other position. So this currently always returns the hand
['Z', 'Y', 'Y', 'X', 'W', 'W', 'V', 'V', 'U', 'U']
Talk about hard mode!
| if (lettersInHandCopy.includes(letter)) { | ||
| let indexHand = lettersInHandCopy.indexOf(letter); | ||
| lettersInHandCopy.splice(indexHand, 1); |
There was a problem hiding this comment.
Both includesand indexOf require iterating through the data. But indexOf returns -1 if the sought value isn't present. So we could omit the includes check like
const letterIndex = lettersInHandCopy.indexOf(letter)
if (letterIndex !== -1) {
lettersInHandCopy.splice(letterIndex, 1)There was a problem hiding this comment.
Observe that we are performing operations (indexOf and splice) which require iterating through the array of data, and that we are in a loop. So we're iterating over the data each time of the outer loop.
Consider building frequency maps for the input and the hand which requires iterating over each only once. Then rather than repeatedly removing letters from the hand array, we could compare the counts of each input letter with the available count in the hand.
| } else if (word.length == "") { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
This condition is unnecessary. If the word is empty, score will be initialized to 0, the loop will not be entered (leaving it 0), and no bonus will be added, still leaving it 0.
|
|
||
| let score = 0; | ||
|
|
||
| for (let letter of word) { |
There was a problem hiding this comment.
Prefer const for the loop variable in a for/of loop.
|
|
||
| export const scoreWord = (word) => { | ||
| // Implement this method for wave 3 | ||
| const lettersDict = { |
There was a problem hiding this comment.
Consider moving this outside the function (or wrap it aggressively) so that the function becomes shorter.
| winner["score"] = score; | ||
| console.log(word, score); | ||
| } | ||
| if (score === winner["score"] && winner["word"].length < 10) { |
There was a problem hiding this comment.
This should be else if. We don't need to consider this if the word flat out beat the current winner.
There was a problem hiding this comment.
Otherwise, nice job teasing apart the tie breakers.
|
|
||
| export const drawLetters = () => { | ||
| // Implement this method for wave 1 | ||
| const letterPool = { |
There was a problem hiding this comment.
Consider moving this outside the function (or wrap it aggressively) so that the function becomes shorter.
2 participants
No description provided.