Ashley_Benson_Seaturtles #116
Conversation
There was a problem hiding this comment.
Good job! There were a few places where you could think about ways to avoid removing things from a list in order to make the code a bit more scalable, but nothing that would pose a problem for the scale of the came data. Your approach is overall clean and easy to understand. The structure of your tie-breaker code was particularly easy to read.
Note, there were two tests in adagrams.test.js that you were meant to fill in (they were throwing hard-coded errors). Keep an eye out for details like that. But when I ran updated versions against your implementations, everything passed fine.
Nice work!
| @@ -110,7 +110,7 @@ Our first job is to build a hand of 10 letters. To do so, implement the function | |||
| - Each string should contain exactly one letter | |||
| - These represent the hand of letters that the player has drawn | |||
| - The letters should be randomly drawn from a pool of letters | |||
| - This letter pool should reflect the distribution of letters as described in the table below | |||
| - This letter pool should reflect the distribution of letters as describe.skipd in the table below | |||
There was a problem hiding this comment.
When doing global search/replace across multiple files, be sure to check for unintended side effects.
| }); | ||
|
|
||
| it("returns a score of 0 if given an empty input", () => { | ||
| throw "Complete test"; |
There was a problem hiding this comment.
This tests should have been completed. Something like
expectScores({"": 0});would work (and your code does pass when I made this change locally).
| const words = ["XXX", "XXXX", "X", "XX"]; | ||
| const correct = { word: "XXXX", score: scoreWord("XXXX") }; | ||
|
|
||
| throw "Complete test by adding an assertion"; |
There was a problem hiding this comment.
This tests should have been completed. Something like
expect(highestScoreFrom(words)).toEqual(correct);would work (and your code does pass when I made this change locally).
| const allLetters = [] | ||
| for (const [letter, frequency] of Object.entries(letterPool)) { | ||
| for (let i = 0; i < frequency; i++) { | ||
| allLetters.push(letter) | ||
| } | ||
| } |
There was a problem hiding this comment.
Nice approach to build up a list of all possible letter tiles.
| const hand = [] | ||
|
|
||
| for (let i = 0; i < 10; i++) { | ||
| const thisLetter = allLetters[Math.floor(Math.random() * allLetters.length)] |
There was a problem hiding this comment.
Rather than getting the letter directly, then need ing to go back and look up where it was later, consider storing the calculated index
const randomPos = Math.floor(Math.random() * allLetters.length);and using that location to both look up the letter to push into the hand, as well as for removing it from the available letter tiles (avoiding the indexOf call).
| score: 0, | ||
| }; | ||
|
|
||
| for (let i = 0; i < n; i++) { |
There was a problem hiding this comment.
Consider iterating over the words in the candidate list directly using for/of
for (const word of words) {
...then replace any references to words[i] with word. This has a much smaller chance of introducing a typo than repeated needing to reference words[i].
| }; | ||
|
|
||
| for (let i = 0; i < n; i++) { | ||
| if (scoreWord(words[i]) > high_score.score) { |
There was a problem hiding this comment.
Calculate the word score once before performing all of these checks
const wordScore = scoreWord(word); // assumes for/of suggestion above
...and then replace scoreWord(word) with wordScore. This has a much smaller chance of introducing a typo than repeated needing to reference scoreWord(word) (or especially scoreWord(words[i])).
| //tie | ||
| } else if (scoreWord(words[i]) === high_score.score) { | ||
| if (high_score.word.length === 10) { | ||
| high_score = high_score; |
There was a problem hiding this comment.
Rather than re-assigning to itself, we could continue to move on to the next word, leaving the current high score unchanged.
| score: scoreWord(words[i]), | ||
| }; | ||
| //tie | ||
| } else if (scoreWord(words[i]) === high_score.score) { |
| Y: 4, | ||
| Z: 10, | ||
| } | ||
| const alphabet = "abcdefghijklmnopqrstuvwxyz".toUpperCase().split(""); |
There was a problem hiding this comment.
This list of characters isn't really needed (see related comment in scoreWord implementation).
No description provided.