C22 Sphinx - Denise C. #39
Conversation
| const handSize = 10 | ||
|
|
||
| const letterPool = { |
There was a problem hiding this comment.
While we use the const keyword to ensure the values cannot be reassigned, we should also use capital letters to convey to other devs the value is fixed.
| const handSize = 10 | |
| const letterPool = { | |
| const HAND_SIZE = 10 | |
| const LETTER_POOL = { |
| 'Z': 1 | ||
| }; | ||
|
|
||
| const letterPointValues = { |
There was a problem hiding this comment.
| const letterPointValues = { | |
| const LETTER_POINT_VALUES = { |
| } | ||
|
|
||
| return listOfAllLetters; | ||
|
|
There was a problem hiding this comment.
Well done! I like that you put this logic in a helper function.
The blank line on line 72 should be removed. Per the Google JS styleguide: "Within method bodies, sparingly to create logical groupings of statements. Blank lines at the start or end of a function body are not allowed."
| const listOfAllLetters = createLetterPool(); | ||
|
|
||
| while (handOfLettersList.length < handSize) { | ||
| const aRandomIndexThatAccessesRandomLetter = Math.floor(Math.random() * listOfAllLetters.length); |
There was a problem hiding this comment.
I like that you don't hardcode a value and use listOfAllLetters.length
| const listOfAllLetters = createLetterPool(); | ||
|
|
||
| while (handOfLettersList.length < handSize) { | ||
| const aRandomIndexThatAccessesRandomLetter = Math.floor(Math.random() * listOfAllLetters.length); |
There was a problem hiding this comment.
I know naming is challenging and your variable name is descriptive, but since it's so long it makes the code a little less readable to me (a subjective opinion). I'd probably name this variable something like randIndex.
| export const drawLetters = () => { | ||
| // Implement this method for wave 1 | ||
| const handOfLettersList = []; |
There was a problem hiding this comment.
👍 Nice job using const for this list. We should always prefer to use const over let whenever possible.
| const aRandomIndexThatAccessesRandomLetter = Math.floor(Math.random() * listOfAllLetters.length); | ||
| const randomLetterChosen = listOfAllLetters[aRandomIndexThatAccessesRandomLetter]; | ||
| handOfLettersList.push(randomLetterChosen); | ||
| listOfAllLetters.splice(aRandomIndexThatAccessesRandomLetter, 1); |
There was a problem hiding this comment.
splice like python's remove has linear time complexity with respect to the length of the list. This won't be a problem for the size of data we're working with, but consider using a strategy of virtually "dividing" the list into a used and unused side. Swapping a value to the used side would be constant time!
| // Implement this method for wave 2 | ||
|
|
||
| export const usesAvailableLetters = (word, handOfLettersList) => { | ||
| const wordBankList = [...handOfLettersList]; |
|
|
||
| } else { | ||
| const index = wordBankList.indexOf(letter); | ||
| wordBankList.splice(index, 1); |
There was a problem hiding this comment.
Like I mentioned above, splice time complexity is linear. What if you created a frequency map and then leveraged the constant look up time of an object?
After you have an object with the key being a letter and value being it's frequency in word you can use that to figure out if you have a valid word from the letters in your hand.
No description provided.